Skip to content

Add flag to strip context #4648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
causten merged 6 commits into from
Apr 24, 2026
+16 −1
Merged

Add flag to strip context #4648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
176a69b
Add a flag to strip context from mxr file in the driver
pfultz2 Jan 27, 2026
baff256
Format
pfultz2 Jan 27, 2026
f9ccda9
Strip targets with context
pfultz2 Jan 27, 2026
a359ba2
Format
pfultz2 Jan 27, 2026
4909973
Update year
pfultz2 Mar 3, 2026
68e13ad
Merge branch 'develop' into strip-context
causten Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/driver/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ struct loader
bool replace_literals = false;
bool brief = false;
bool verbose = false;
bool strip_context = false;
bool use_debug_symbols = false;
std::string output_type;
std::string output;
Expand Down Expand Up @@ -255,6 +256,10 @@ struct loader
ap.help("Replace literals with parameters"),
ap.set_value(true));
ap(passes, {"--apply-pass", "-p"}, ap.help("Passes to apply to model"), ap.append());
ap(strip_context,
{"--strip-context"},
ap.help("Strip context from program"),
ap.set_value(true));
Comment on lines +259 to +262
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new --strip-context flag is user-facing but there’s no corresponding documentation update (driver option docs under docs/driver/*.rst) and the PR template indicates a CHANGELOG.md entry is required when the “Added” category is selected. Please add the doc entry for the new flag and a changelog entry describing the new behavior.

Copilot uses AI. Check for mistakes.
ap(output_type,
{"--graphviz", "-g"},
ap.help("Print out a graphviz representation."),
Expand Down Expand Up @@ -470,6 +475,8 @@ struct loader
{
trim_module(*p.get_main_module(), trim, trim_size);
}
if(strip_context)
p.clear_context();
Comment on lines +478 to +479
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The driver help text says “Strip context from program”, but the implementation calls p.clear_context() which currently clears both contexts and targets. Either adjust the help text to reflect that it removes compilation state (contexts + targets), or change the underlying API to only remove contexts.

Copilot uses AI. Check for mistakes.
if(replace_literals)
{
replace_literals_with_params(p);
Expand Down
4 changes: 3 additions & 1 deletion src/include/migraphx/program.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* The MIT License (MIT)
*
* Copyright (c) 2015-2025 Advanced Micro Devices, Inc. All rights reserved.
* Copyright (c) 2015-2026 Advanced Micro Devices, Inc. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -95,6 +95,8 @@ struct MIGRAPHX_EXPORT program

context& get_context() const;

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new API name clear_context() is misleading because it also clears impl->targets. If the intent is to strip all compilation state, consider renaming to something like clear_compilation_state()/strip_compilation_state() or documenting that targets are cleared as well; otherwise, only clear contexts here and leave target management to a separate API.

Suggested change
/**
* @brief Clears cached compilation state associated with this program.
*
* This resets the program context and also clears any stored targets.
*/

Copilot uses AI. Check for mistakes.
void clear_context();

instruction_ref validate() const;

target_assignments get_target_assignments(const std::vector<target>& targets,
Expand Down
6 changes: 6 additions & 0 deletions src/program.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ context& program::get_context() const
return impl->contexts.front();
}

void program::clear_context()
{
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

program::clear_context() clears impl->contexts/impl->targets but leaves each instruction’s target_id unchanged. If a loaded MXR was multi-target compiled (so some target_id > 0), using --strip-context and then recompiling with the single-target program::compile(const target&) can later index contexts[ins->get_target_id()] out of bounds during module::finalize, causing a crash. Consider resetting all instruction target ids to 0 (e.g., run a pass like mark_instruction_target{0} over all modules) as part of clear_context() (or in compile(const target&) before finalization).

Suggested change
{
{
for(auto& m : impl->modules)
mark_instruction_target{0}.apply(m.second);

Copilot uses AI. Check for mistakes.
impl->contexts.clear();
impl->targets.clear();
}

instruction_ref program::validate() const
{
const auto* mm = this->get_main_module();
Expand Down
Loading