Skip to content

[CIR][NFC] Change GetMemberOp index type #1811

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.

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 1 commit into from
Aug 15, 2025

Conversation

Andres-Salamanca
Copy link
Contributor

This PR is inspired by the discussion in #1745 (comment).
When changing the type of

IndexAttr:$index,
I64Attr:$index

in GetMemberOp, the getIndex method becomes auto-generated.
This allows us to remove the custom builder previously defined for this operation.

@Andres-Salamanca
Copy link
Contributor Author

I haven’t looked exactly, but I think this could also be done for:

ExtractMemberOp:

let extraClassDeclaration = [{
/// Get the index of the record member being accessed.
uint64_t getIndex() { return getIndexAttr().getZExtValue(); }
}];

And for InsertMemberOp:
let extraClassDeclaration = [{
/// Get the index of the record member being accessed.
uint64_t getIndex() { return getIndexAttr().getZExtValue(); }

I can add this in a future PR.

@tommymcm
Copy link
Contributor

tommymcm commented Aug 13, 2025

Would this impact targets where intptr_t is not 64 bits? Or is there a guarantee somewhere that says this will always be within 64 bits? Is the same true for {Extract,Insert}Member?

@andykaylor
Copy link
Collaborator

Would this impact targets where intptr_t is not 64 bits? Or is there a guarantee somewhere that says this will always be within 64 bits? Is the same true for {Extract,Insert}Member?

That's not a bad question, but since we were always converting the IndexAttr to a int64_t value during lowering, this isn't introducing a new problem. For GetMemberOp in particular, we are indexing into a structure, and when that gets lowered to LLVM IR, the index has to be an i32 value.

@Andres-Salamanca
Copy link
Contributor Author

In both InsertMember and ExtractMember lowering we already cast the field index to std::int64_t:

mlir::LogicalResult CIRToLLVMInsertMemberOpLowering::matchAndRewrite(
cir::InsertMemberOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
std::int64_t indecies[1] = {static_cast<std::int64_t>(op.getIndex())};

mlir::LogicalResult CIRToLLVMExtractMemberOpLowering::matchAndRewrite(
cir::ExtractMemberOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
std::int64_t indecies[1] = {static_cast<std::int64_t>(op.getIndex())};

So, at lowering time, we’re explicitly on 64-bit regardless.

And in the definition of I64Attr, it uses uint64_t:

def I64Attr : TypedSignlessIntegerAttrBase<
    I64, "uint64_t", "64-bit signless integer attribute">;

The IntegerType definition in MLIR also explicitly states that the bit-width is designated:
https://github.com/llvm/llvm-project/blob/177f27d22092cb64e871e6cd2f8981d24e823186/mlir/include/mlir/IR/BuiltinTypes.td#L515-L516

@bcardosolopes bcardosolopes merged commit abcff7b into llvm:main Aug 15, 2025
7 of 10 checks passed
bcardosolopes pushed a commit that referenced this pull request Aug 18, 2025
This PR, similar to #1811 changes
the index type for `ExtractMemberOp` and `InsertMemberOp`. This
modification enables automatic generation of the getIndex method and
removes the need for a custom builder.
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