Skip to content

Modernize pass instrument#3395

Open
juhyunbae17 wants to merge 5 commits intoonnx:mainfrom
juhyunbae17:modernize_pass_instrument
Open

Modernize pass instrument#3395
juhyunbae17 wants to merge 5 commits intoonnx:mainfrom
juhyunbae17:modernize_pass_instrument

Conversation

@juhyunbae17
Copy link
Collaborator

This PR is to modernize Instrument pass. Following is included

  • definition of options moves to .td and options will be generated by TableGen
  • change multi-positional createInstrumentPass() to single positional so that its declaration and pass registration code can be generated by TableGen instead of having explicit declaration of it alongside TableGen already used in this Instrument Pass.

Signed-off-by: Juhyun Bae <juhyun.bae@ibm.com>
Signed-off-by: Juhyun Bae <juhyun.bae@ibm.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@chentong319
Copy link
Collaborator

Yes, this PR is the right direction.
One thing I am not sure whether we should use InstrumentPassOption directly, or create a new createInstrumentPass function to hide the details of construction of InstrumentPassOption. In MLIR, both approaches are used in different dialect. What's your opinion?

@juhyunbae17
Copy link
Collaborator Author

If the concern is about too many pass options exposed (perhaps unnecessarily) to user and help messages, it seems there is a way we could hide certain options with following flag in the .td file

In .td file,

def MyPass : Pass<"my-pass"> {
  let summary = "Example pass with a hidden option";
  let options = [
    Option<"myOption", "my-option", "bool", /*default=*/"false",
           "A description of the option",
           "llvm::cl::Hidden"> // <--- This hides it from standard --help
  ];
}

I don't know if others have second opinion.

Copy link
Member

@tungld tungld left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the update!

@tungld
Copy link
Member

tungld commented Mar 2, 2026

@jenkins-droid test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants