Skip to content

[mlir-tblgen] Only create body for unpruned create #166019

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
merged 3 commits into from
Nov 2, 2025
Merged

[mlir-tblgen] Only create body for unpruned create #166019

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b118bd3
[mlir-tblgen] Do not create body for pruned create
hsjts0u Nov 1, 2025
ade6b77
Add test
hsjts0u Nov 2, 2025
cafd448
Revert "Add test"
hsjts0u Nov 2, 2025
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
12 changes: 7 additions & 5 deletions mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2632,11 +2632,13 @@ void OpEmitter::genInlineCreateBody(
interleaveComma(nonBuilderStateArgsList, nonBuilderStateArgsOS);
nonBuilderStateArgs = ", " + nonBuilderStateArgs;
}
cWithLoc->body() << llvm::formatv(inlineCreateBody, locParamName,
nonBuilderStateArgs,
opClass.getClassName());
cImplicitLoc->body() << llvm::formatv(inlineCreateBodyImplicitLoc,
nonBuilderStateArgs);
if (cWithLoc)
cWithLoc->body() << llvm::formatv(inlineCreateBody, locParamName,
nonBuilderStateArgs,
opClass.getClassName());
if (cImplicitLoc)
cImplicitLoc->body() << llvm::formatv(inlineCreateBodyImplicitLoc,
nonBuilderStateArgs);
Comment on lines +2635 to +2641
Copy link
Contributor

@makslevental makslevental Nov 1, 2025

Choose a reason for hiding this comment

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

I don't understand - when are these null? when does addStaticMethod return a null?

Copy link
Contributor

@makslevental makslevental Nov 1, 2025

Choose a reason for hiding this comment

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

ah it calls

  /// Add a new method if it is not made redundant by any existing methods and
  /// prune and existing methods made redundant.
  Method *addMethodAndPrune(Method &&newMethod);

gotcha

Copy link
Contributor

@makslevental makslevental Nov 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@hsjts0u one question: is the header (decl) still being generated or is the header also being pruned (sorry I don't remember)...

Copy link
Contributor Author

@hsjts0u hsjts0u Nov 1, 2025

Choose a reason for hiding this comment

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

I'm honestly not sure, but it looks like the decls and defs are generated at the construction of the OpEmitterClass and the actual emit methods are methods of the class so I assume both are pruned.

Copy link
Contributor

@makslevental makslevental Nov 1, 2025

Choose a reason for hiding this comment

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

Ok I haven't checked yet but I'll check soon

Copy link
Contributor

@makslevental makslevental Nov 1, 2025

Choose a reason for hiding this comment

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

somewhere in here should be fine https://github.com/hsjts0u/llvm-project/blob/main/mlir/test/mlir-tblgen/op-decl-and-defs.td

Copy link
Contributor Author

@hsjts0u hsjts0u Nov 2, 2025

Choose a reason for hiding this comment

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

I can repro but it is kind of awkward. I hit this https://github.com/hsjts0u/llvm-project/blob/main/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp#L3121 before I hit the segfault. Something like this would segfault if that line is commented out or the if (body) check is added back.

def ReproOp : NS_Op<"repro", []> {
  let arguments = (ins
    SymbolNameAttr:$sym_name,
    TypeAttrOf<FunctionType>:$function_type,
    OptionalAttr<DictArrayAttr>:$arg_attrs,
    OptionalAttr<DictArrayAttr>:$res_attrs,
    OptionalAttr<UnitAttr>:$unit
  );
  let regions = (region AnyRegion:$body);

  let builders = [
  OpBuilder<(ins
    "StringRef":$name, "FunctionType":$type,
    CArg<"ArrayRef<DictionaryAttr>", "{}">:$argAttrs,
    CArg<"ArrayRef<DictionaryAttr>", "{}">:$resultAttrs)
  >,
  OpBuilder<(ins
    "StringRef":$name, "FunctionType":$type)
  >];
}

Copy link
Contributor Author

@hsjts0u hsjts0u Nov 2, 2025

Choose a reason for hiding this comment

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

Added back the if (body) statement as it seems to be checking when it is not necessary for it to check. Even the tests that were added with the removal of if (body) still pass when added back.

Copy link
Contributor

@makslevental makslevental Nov 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

okay cool your snippet here led me to be able to repro your segfault so you can revert the change you made in order to add the test (and the test itself) and then i'll merge.

Copy link
Contributor Author

@hsjts0u hsjts0u Nov 2, 2025

Choose a reason for hiding this comment

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

Thanks! Reverted

}

void OpEmitter::genSeparateArgParamBuilder() {
Expand Down