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

[myhsu]

Summary

Constants in MLIR are not globally unique, unlike in LLVM IR. Therefore, reusing previously-translated constants might create operations that are not dominated by the constant (because the previously-translated ones can be placed anywhere)

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

(Note that the test case attached looks really similar to that in D124402, but the indices in ConstantExpr-GEP are different: in this patch all of them are zeros. Without this patch, the llvm.getelementptr translated from the second ConstantExpr-GEP will try to use llvm.constant(0: i32)-s that are below itself, because we're reusing the cached ConstantInt-type zero value translated earlier)

[myhsu]

Rebase

[ftynse]

I edited the description a bit. Please also note that phabricator-style markup (e.g., using // for italic) looks weird in git commit messages (e.g., C-style comment).

even another llvm.func!

Isn't the cache cleared on entering a new function? The error is only manifest for globals.

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

5–24

Could you please describe in text what was happening before this patch? This will help to differentiate regressions from other kinds of test breakages in the future.

[myhsu]

add bug descriptions into the test file.

[myhsu]

In D124404#3473986, @ftynse wrote:

I edited the description a bit. Please also note that phabricator-style markup (e.g., using // for italic) looks weird in git commit messages (e.g., C-style comment).

even another llvm.func!

Isn't the cache cleared on entering a new function? The error is only manifest for globals.

Yes you're right, I'd removed this from description / commit message.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Value Importer::processConstant(llvm::Constant *c) {
   OpBuilder bEntry(currentEntryBlock, currentEntryBlock->begin());
   if (Attribute attr = getConstantAsAttr(c)) {
     // These constants can be represented as attributes.
     OpBuilder b(currentEntryBlock, currentEntryBlock->begin());
     Type type = processType(c->getType());
     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),
                                       processGlobal(gv));
 
   if (auto *ce = dyn_cast<llvm::ConstantExpr>(c)) {
     llvm::Instruction *i = ce->getAsInstruction();
     OpBuilder::InsertionGuard guard(b);
     b.setInsertionPoint(currentEntryBlock, currentEntryBlock->begin());
     if (failed(processInstruction(i)))
       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] = 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)) {
     unsigned numElements = c->getNumOperands();
     std::function<llvm::Constant *(unsigned)> getElement =
         [&](unsigned index) -> llvm::Constant * {
       return c->getAggregateElement(index);
     };

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

+; 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
+@str0 = private unnamed_addr constant [5 x i8] c"aaaa\00"
+@str1 = private unnamed_addr constant [5 x i8] c"bbbb\00"
+@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)}]
+