[Mlir-commits] [mlir] a75657d - [mlir][LLVMIR] Do not cache llvm::Constant into instMap

[Min-Yih Hsu]

Author: Min-Yih Hsu
Date: 2022-04-27T09:43:49-07:00
New Revision: a75657d66a1245f78d58c8cb5ded042a13a58299

URL: https://github.com/llvm/llvm-project/commit/a75657d66a1245f78d58c8cb5ded042a13a58299
DIFF: https://github.com/llvm/llvm-project/commit/a75657d66a1245f78d58c8cb5ded042a13a58299.diff

LOG: [mlir][LLVMIR] Do not cache llvm::Constant into instMap

Constants in MLIR are not globally unique, unlike that in LLVM IR.
Therefore, reusing previous-translated constants might cause the user
operations not being dominated by the constant (because the
previous-translated ones can be placed in arbitrary place)

This indeed misses some opportunities where we actually can reuse a
previous-translated constants, but verbosity is not our first priority
here.

Differential Revision: https://reviews.llvm.org/D124404

Added: 
    mlir/test/Target/LLVMIR/Import/incorrect-constant-caching.ll

Modified: 
    mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
index af9a2cf666a9..90c3d63b65f0 100644
--- a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -465,15 +465,14 @@ Value Importer::processConstant(llvm::Constant *c) {
     if (!type)
       return nullptr;
     if (auto symbolRef = attr.dyn_cast<FlatSymbolRefAttr>())
-      return instMap[c] = bEntry.create<AddressOfOp>(unknownLoc, type,
-                                                     symbolRef.getValue());
-    return instMap[c] = bEntry.create<ConstantOp>(unknownLoc, type, attr);
+      return bEntry.create<AddressOfOp>(unknownLoc, type, symbolRef.getValue());
+    return bEntry.create<ConstantOp>(unknownLoc, type, attr);
   }
   if (auto *cn = dyn_cast<llvm::ConstantPointerNull>(c)) {
     Type type = processType(cn->getType());
     if (!type)
       return nullptr;
-    return instMap[c] = bEntry.create<NullOp>(unknownLoc, type);
+    return bEntry.create<NullOp>(unknownLoc, type);
   }
   if (auto *gv = dyn_cast<llvm::GlobalVariable>(c))
     return bEntry.create<AddressOfOp>(UnknownLoc::get(context),
@@ -498,13 +497,13 @@ Value Importer::processConstant(llvm::Constant *c) {
     // Remove this zombie LLVM instruction now, leaving us only with the MLIR
     // op.
     i->deleteValue();
-    return instMap[c] = value;
+    return value;
   }
   if (auto *ue = dyn_cast<llvm::UndefValue>(c)) {
     Type type = processType(ue->getType());
     if (!type)
       return nullptr;
-    return instMap[c] = bEntry.create<UndefOp>(UnknownLoc::get(context), type);
+    return bEntry.create<UndefOp>(UnknownLoc::get(context), type);
   }
 
   if (isa<llvm::ConstantAggregate>(c) || isa<llvm::ConstantAggregateZero>(c)) {

diff  --git a/mlir/test/Target/LLVMIR/Import/incorrect-constant-caching.ll b/mlir/test/Target/LLVMIR/Import/incorrect-constant-caching.ll
new file mode 100644
index 000000000000..411a8adf98df
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/Import/incorrect-constant-caching.ll
@@ -0,0 +1,34 @@
+; RUN: mlir-translate --import-llvm %s | FileCheck %s
+
+; Testing the fix for issue where llvm.getelementptr translated from the second
+; ConstantExpr-GEP tried to use llvm.constant(0: i32)-s that were below itself,
+; because it's reusing the cached ConstantInt-type zero value translated earlier.
+
+; This test is primarily used to make sure an verification is passed. Thus, we
+; only wrote minimum level of checks.
+
+%my_struct = type {i32, i8*}
+; CHECK: llvm.mlir.constant(0 : i32) : i32
+; CHECK: llvm.mlir.constant(0 : i32) : i32
+; CHECK: llvm.mlir.addressof @str1 : !llvm.ptr<array<5 x i8>>
+; CHECK: llvm.getelementptr
+; CHECK: llvm.mlir.constant(7 : i32) : i32
+; CHECK: llvm.mlir.undef : !llvm.struct<"my_struct", (i32, ptr<i8>)>
+; CHECK: llvm.insertvalue
+; CHECK: llvm.insertvalue
+; CHECK: llvm.mlir.constant(0 : i32) : i32
+; CHECK: llvm.mlir.constant(0 : i32) : i32
+; CHECK: llvm.mlir.addressof @str0 : !llvm.ptr<array<5 x i8>>
+; CHECK: llvm.getelementptr
+; CHECK: llvm.mlir.constant(8 : i32) : i32
+; CHECK: llvm.mlir.undef : !llvm.struct<"my_struct", (i32, ptr<i8>)>
+; CHECK: llvm.insertvalue
+; CHECK: llvm.insertvalue
+; CHECK: llvm.mlir.undef : !llvm.array<2 x struct<"my_struct", (i32, ptr<i8>)>>
+; CHECK: llvm.insertvalue
+; CHECK: llvm.insertvalue
+; CHECK: llvm.return
+ at str0 = private unnamed_addr constant [5 x i8] c"aaaa\00"
+ at str1 = private unnamed_addr constant [5 x i8] c"bbbb\00"
+ at g = global [2 x %my_struct] [%my_struct {i32 8, i8* getelementptr ([5 x i8], [5 x i8]* @str0, i32 0, i32 0)}, %my_struct {i32 7, i8* getelementptr ([5 x i8], [5 x i8]* @str1, i32 0, i32 0)}]
+