Skip to content

[mlir][amd] fix LLVM::InsertValueOp::create failure to disambiguate #150605

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
Jul 25, 2025

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Jul 25, 2025

fixes #149879 (comment)

Note this happens because ADL can't disambiguate between mlir::DenseI64ArrayAttr and llvm::ArrayRef<int64_t> for the value 0 which I guess is equal to nullptr on some (most?) systems.

Note, this only occurs with the value 0.

@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-backend-amdgpu

Author: Maksim Levental (makslevental)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/150605.diff

1 Files Affected:

  • (modified) mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp (+2-2)
diff --git a/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp b/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
index bc0d9bf2ea771..b6f6167d1dfb3 100644
--- a/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
+++ b/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
@@ -232,8 +232,8 @@ struct FatRawBufferCastLowering
     Value result = MemRefDescriptor::poison(
         rewriter, loc,
         getTypeConverter()->convertType(op.getResult().getType()));
-    result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr,
-                                         kAllocatedPtrPosInMemRefDescriptor);
+    SmallVector<int64_t> pos{kAllocatedPtrPosInMemRefDescriptor};
+    result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr, pos);
     result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr,
                                          kAlignedPtrPosInMemRefDescriptor);
     result = LLVM::InsertValueOp::create(rewriter, loc, result, offset,

@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2025

@llvm/pr-subscribers-mlir-gpu

Author: Maksim Levental (makslevental)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/150605.diff

1 Files Affected:

  • (modified) mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp (+2-2)
diff --git a/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp b/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
index bc0d9bf2ea771..b6f6167d1dfb3 100644
--- a/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
+++ b/mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
@@ -232,8 +232,8 @@ struct FatRawBufferCastLowering
     Value result = MemRefDescriptor::poison(
         rewriter, loc,
         getTypeConverter()->convertType(op.getResult().getType()));
-    result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr,
-                                         kAllocatedPtrPosInMemRefDescriptor);
+    SmallVector<int64_t> pos{kAllocatedPtrPosInMemRefDescriptor};
+    result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr, pos);
     result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr,
                                          kAlignedPtrPosInMemRefDescriptor);
     result = LLVM::InsertValueOp::create(rewriter, loc, result, offset,

@makslevental makslevental changed the title [mlir][amd] fix LLVM::InsertValueOp::create [mlir][amd] fix LLVM::InsertValueOp::create failure to disambiguate Jul 25, 2025
@makslevental
Copy link
Contributor Author

I don't have a windows machine so I'm gonna wait for CI to complete just to be sure...

@makslevental makslevental merged commit 8e8f195 into llvm:main Jul 25, 2025
13 checks passed
@makslevental makslevental deleted the makslevental/fix-llvm-create branch July 25, 2025 11:56
result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr,
kAllocatedPtrPosInMemRefDescriptor);
SmallVector<int64_t> pos{kAllocatedPtrPosInMemRefDescriptor};
result = LLVM::InsertValueOp::create(rewriter, loc, result, fatPtr, pos);
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to do ArrayRef{kAllocated...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you have have to do ArrayRef<int64_t>{...} and I thought this was version was clearer but yea you're right.

@DavidSpickett
Copy link
Collaborator

The 2 stage build will take ages, but this fixed https://lab.llvm.org/buildbot/#/builders/207/builds/4343 as well so we know it works. Thanks!

A bit confused why you only had to change the first one, but if it works it works.

@makslevental
Copy link
Contributor Author

A bit confused why you only had to change the first one, but if it works it works.

It's because the ADL failure is due to 0 == nullptr and both ArrayRef, DenseI64ArrayAttr are "reference" types. For the rest of the callsites the "pos" arg is non-zero and so ADL works and correctly selects the ArrayRef<int64_t> position overload.

@DavidSpickett
Copy link
Collaborator

Oh right there's 3 different constants being used, I read them as all being the same.

mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
…lvm#150605)

fixes
llvm#149879 (comment)

Note this happens because ADL can't disambiguate between
`mlir::DenseI64ArrayAttr` and `llvm::ArrayRef<int64_t>` **for the value
0** which I guess is equal to nullptr on some (most?) systems.

Note, this only occurs with the value 0.
ajaden-codes pushed a commit to Jaddyen/llvm-project that referenced this pull request Jul 28, 2025
…lvm#150605)

fixes
llvm#149879 (comment)

Note this happens because ADL can't disambiguate between
`mlir::DenseI64ArrayAttr` and `llvm::ArrayRef<int64_t>` **for the value
0** which I guess is equal to nullptr on some (most?) systems.

Note, this only occurs with the value 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants