[mlir][LLVM] Use int32_t to indirectly construct GEPArg

[andrey-golubev]

GEPArg can only be constructed from int32_t and mlir::Value. Explicitly cast other types (e.g. unsigned, size_t) to int32_t to avoid narrowing conversion warnings on MSVC. Some recent examples of such are:

mlir\lib\Dialect\LLVMIR\Transforms\TypeConsistency.cpp: error C2398: Element '1': conversion from 'size_t' to 'T' requires a narrowing conversion
with
[
T=mlir::LLVM::GEPArg
]

mlir\lib\Dialect\LLVMIR\Transforms\TypeConsistency.cpp: error C2398: Element '1': conversion from 'unsigned int' to 'T' requires a narrowing conversion
with
[
T=mlir::LLVM::GEPArg
]

[llvmbot]

@llvm/pr-subscribers-mlir-gpu
@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Andrei Golubev (andrey-golubev)

Changes

GEPArg can only be constructed from int32_t and mlir::Value. Explicitly cast other types (e.g. unsigned, size_t) to int32_t to avoid narrowing conversion warnings on MSVC. Some recent examples of such are:

mlir\lib\Dialect\LLVMIR\Transforms\TypeConsistency.cpp: error C2398: Element '1': conversion from 'size_t' to 'T' requires a narrowing conversion
with
[
T=mlir::LLVM::GEPArg
]

mlir\lib\Dialect\LLVMIR\Transforms\TypeConsistency.cpp: error C2398: Element '1': conversion from 'unsigned int' to 'T' requires a narrowing conversion
with
[
T=mlir::LLVM::GEPArg
]

---

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

3 Files Affected:

• (modified) mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp (+2-1)
• (modified) mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp (+5-4)
• (modified) mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp (+5-4)

diff --git a/mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp b/mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp
index ae2bd8e5b5405d..73d418cb841327 100644
--- a/mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp
+++ b/mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp
@@ -529,7 +529,8 @@ LogicalResult GPUPrintfOpToVPrintfLowering::matchAndRewrite(
                                       /*alignment=*/0);
   for (auto [index, arg] : llvm::enumerate(args)) {
     Value ptr = rewriter.create<LLVM::GEPOp>(
-        loc, ptrType, structType, tempAlloc, ArrayRef<LLVM::GEPArg>{0, index});
+        loc, ptrType, structType, tempAlloc,
+        ArrayRef<LLVM::GEPArg>{0, static_cast<int32_t>(index)});
     rewriter.create<LLVM::StoreOp>(loc, arg, ptr);
   }
   std::array<Value, 2> printfArgs = {stringStart, tempAlloc};
diff --git a/mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp b/mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
index f853d5c47b623c..78d4e806246872 100644
--- a/mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
+++ b/mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
@@ -1041,13 +1041,14 @@ Value ConvertLaunchFuncOpToGpuRuntimeCallPattern::generateParamsArray(
   auto arrayPtr = builder.create<LLVM::AllocaOp>(
       loc, llvmPointerType, llvmPointerType, arraySize, /*alignment=*/0);
   for (const auto &en : llvm::enumerate(arguments)) {
+    const auto index = static_cast<int32_t>(en.index());
     Value fieldPtr =
         builder.create<LLVM::GEPOp>(loc, llvmPointerType, structType, structPtr,
-                                    ArrayRef<LLVM::GEPArg>{0, en.index()});
+                                    ArrayRef<LLVM::GEPArg>{0, index});
     builder.create<LLVM::StoreOp>(loc, en.value(), fieldPtr);
-    auto elementPtr = builder.create<LLVM::GEPOp>(
-        loc, llvmPointerType, llvmPointerType, arrayPtr,
-        ArrayRef<LLVM::GEPArg>{en.index()});
+    auto elementPtr =
+        builder.create<LLVM::GEPOp>(loc, llvmPointerType, llvmPointerType,
+                                    arrayPtr, ArrayRef<LLVM::GEPArg>{index});
     builder.create<LLVM::StoreOp>(loc, fieldPtr, elementPtr);
   }
   return arrayPtr;
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp
index 72f9295749a66b..b25c831bc7172a 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp
@@ -488,7 +488,8 @@ static void splitVectorStore(const DataLayout &dataLayout, Location loc,
     // Other patterns will turn this into a type-consistent GEP.
     auto gepOp = rewriter.create<GEPOp>(
         loc, address.getType(), rewriter.getI8Type(), address,
-        ArrayRef<GEPArg>{storeOffset + index * elementSize});
+        ArrayRef<GEPArg>{
+            static_cast<int32_t>(storeOffset + index * elementSize)});
 
     rewriter.create<StoreOp>(loc, extractOp, gepOp);
   }
@@ -524,9 +525,9 @@ static void splitIntegerStore(const DataLayout &dataLayout, Location loc,
 
     // We create an `i8` indexed GEP here as that is the easiest (offset is
     // already known). Other patterns turn this into a type-consistent GEP.
-    auto gepOp =
-        rewriter.create<GEPOp>(loc, address.getType(), rewriter.getI8Type(),
-                               address, ArrayRef<GEPArg>{currentOffset});
+    auto gepOp = rewriter.create<GEPOp>(
+        loc, address.getType(), rewriter.getI8Type(), address,
+        ArrayRef<GEPArg>{static_cast<int32_t>(currentOffset)});
     rewriter.create<StoreOp>(loc, valueToStore, gepOp);
 
     // No need to care about padding here since we already checked previously

[andrey-golubev]

@Dinistro @zero9178 @ThomasRaoux please take a look!

CC @joker-eph

[andrey-golubev]

Also, how do I backport this to 18.x properly? Should I retarget the PR to 18.x?

[Dinistro]

Also, how do I backport this to 18.x properly? Should I retarget the PR to 18.x?

MLIR is not part of the release cycle, as far as I know. So no need to do that. Correct me if I'm wrong, @joker-eph.

[Dinistro]

Thanks for the change. Explicit casting does indeed make sense for these cases. LGTM % one nit.

For future PRs, just request review when you want a set of people to take a look.

[zero9178]

Also, how do I backport this to 18.x properly? Should I retarget the PR to 18.x?

See https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches and #79511 as an example. In short the process is to just land the change in main, open an issue, add it to the release milestone and use the cherry pick command. This will automatically create a PR to separately consider whether to backport the PR rather than just landing it in main.

[andrey-golubev]

For future PRs, just request review when you want a set of people to take a look.

Do you mean via GitHub "reviewers"? I think this is only possible for people who have commit access and I don't (yet) :| so have to ping you explicitly :p

[Dinistro]

Do you mean via GitHub "reviewers"? I think this is only possible for people who have commit access and I don't (yet) :| so have to ping you explicitly :p

That's a bit odd, but what ever. Should we merge this for you then once the Windows build bot is done?

[andrey-golubev]

Do you mean via GitHub "reviewers"? I think this is only possible for people who have commit access and I don't (yet) :| so have to ping you explicitly :p

That's a bit odd, but what ever. Should we merge this for you then once the Windows build bot is done?

yes, please!

[Dinistro]

I'll merge this now, as the Windows build bot will probably never get a runner...

[andrey-golubev]

I'll merge this now, as the Windows build bot will probably never get a runner...

Thank you! In fact, i think it will but after a while (iirc i had it finished after a day or so once).