[Mlir-commits] [mlir] ea9bcb8 - [mlir][LLVMIR] Do not cache Instruction generated on-the-fly

[Min-Yih Hsu]

Author: Min-Yih Hsu
Date: 2022-04-27T09:42:59-07:00
New Revision: ea9bcb8b274ae6ef2fd5cbf8c8c9f42a8aa3c096

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

LOG: [mlir][LLVMIR] Do not cache Instruction generated on-the-fly

More specifically, the llvm::Instruction generated by
llvm::ConstantExpr::getAsInstruction. Such Instruction will be deleted
right away, but it's possible that when getAsInstruction is called
again, it will create a new Instruction that has the same address with
the one we just deleted. Thus, we shouldn't keep it in the `instMap` to
avoid a conflicting index that triggers an assertion in
processInstruction.

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

Added: 
    mlir/test/Target/LLVMIR/Import/incorrect-constexpr-inst-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 fc96e7206b61..af9a2cf666a9 100644
--- a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -487,10 +487,18 @@ Value Importer::processConstant(llvm::Constant *c) {
       return nullptr;
     assert(instMap.count(i));
 
+    // If we don't remove entry of `i` here, it's totally possible that the
+    // next time llvm::ConstantExpr::getAsInstruction is called again, which
+    // always allocates a new Instruction, memory address of the newly
+    // created Instruction might be the same as `i`. Making processInstruction
+    // falsely believe that the new Instruction has been processed before
+    // and raised an assertion error.
+    Value value = instMap[i];
+    instMap.erase(i);
     // Remove this zombie LLVM instruction now, leaving us only with the MLIR
     // op.
     i->deleteValue();
-    return instMap[c] = instMap[i];
+    return instMap[c] = value;
   }
   if (auto *ue = dyn_cast<llvm::UndefValue>(c)) {
     Type type = processType(ue->getType());

diff  --git a/mlir/test/Target/LLVMIR/Import/incorrect-constexpr-inst-caching.ll b/mlir/test/Target/LLVMIR/Import/incorrect-constexpr-inst-caching.ll
new file mode 100644
index 000000000000..edc837906745
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/Import/incorrect-constexpr-inst-caching.ll
@@ -0,0 +1,31 @@
+; RUN: mlir-translate --import-llvm %s | FileCheck %s
+; REQUIRES: asserts
+
+; This test is primarily used to make sure an assertion is not triggered.
+; Thus, we only wrote minimum level of checks.
+
+%my_struct = type {i32, i8*}
+; CHECK: llvm.mlir.constant(3 : i32) : i32
+; CHECK: llvm.mlir.constant(2 : 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(1 : 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 1)}, %my_struct {i32 7, i8* getelementptr ([5 x i8], [5 x i8]* @str1, i32 2, i32 3)}]
+