Skip to content

Conversation

Andres-Salamanca
Copy link
Contributor

This PR adds the new attribute CIR_BlockAddrInfoAttr, requested in #1909, which is used in BlockAddressOp and LabelOp. I also created a custom builder to simplify construction so that we don’t have to call mlir::FlatSymbolRefAttr::get and mlir::StringAttr::get every time.

@Andres-Salamanca Andres-Salamanca changed the title Block addr info attr [CIR] Add BlockAddrInfoAttr for LabelOp and BlockAddressOp Sep 24, 2025
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, you probably need to add [NFC] to the title as well.

An identifier which may be referred by cir.goto operation
}];
let arguments = (ins StrAttr:$label);
let arguments = (ins CIR_BlockAddrInfoAttr:$label);
Copy link
Member

@bcardosolopes bcardosolopes Sep 25, 2025

Choose a reason for hiding this comment

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

Feels a redundant to store the entire CIR_BlockAddrInfoAttr given that the function_name will always be the function where the cir.label resides. You could have a method here in extraClassDeclaration that could just return a newly built CIR_BlockAddrInfoAttr, e.g.

BlockAddrInfoAttr getBlockAddressInfo() {
  StringRef fnName = ... // this->getAnyParentOf<FuncOp>(...)
  return BlockAddrInfoAttr::get(fnName, getLabel());
}

If it makes things easier, you could also create a CIR_LabelAttr (which is basically a StrAttr), but not clear if that brings any advantages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the changes to LabelOp. I don’t think it’s worth introducing an extra function, since calling cir::BlockAddrInfoAttr::get is sufficient. and in the CGF we have curFun, so we can get the funcName from there.

@bcardosolopes bcardosolopes merged commit 48a2570 into llvm:main Sep 29, 2025
9 checks passed
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.

2 participants