[mlir][llvm] Use cached constants when importing landingpad instructions

[victor-eds]

Summary

Not using cached constants when importing instructions may lead to
undesired results, as breaking dominance rules in the translated MLIR
module.

Signed-off-by: Victor Perez <[e-mail]>

[zero9178]

LGTM!

[gysit]

Thanks for fixing!

It definitely makes sense to use the cached value directly. Do you by chance understand why convertConstantExpr fails? It seems like there may be an issue with the
convertConstantExpr function itself?

[victor-eds]

@gysit, @zero9178,

Thanks for the quick reviews!

In D149247#4298482, @gysit wrote:

It definitely makes sense to use the cached value directly. Do you by chance understand why convertConstantExpr fails? It seems like there may be an issue with the
convertConstantExpr function itself?

I don't think that's a convertConstantExpr issue. Let me explain why this change is needed looking at the test:

entry:
  ; Constant insertion point = nullptr
  call void @f0(ptr null)  ; %0 = llvm.mlir.null : llvm.ptr.     Constant insertion point = %0
  call void @f1(i32 0)     ; %1 = llvm.mlir.constant 0 : i32. Constant insertion point = %1
  invoke void @f0(ptr null)
      to label %exit unwind label %catch

catch:
  %lp = landingpad { ptr, i32 } catch ptr null  ; No new constant created, but the call to convertConstantExpr updates the constant insertion point to the operation defining the queried constant (ptr null). Constant insertion point = %0
  call void @f2({ptr, i32} {ptr null, i32 0})        ; Constant is defined as a series of llvm.insertvalue operations. These access the null and 0 constants defined before. 0 is defined after the insertion point of constants builder, so it breaks dominance (we'll have %n-1 = llvm.insertvalue %n, ...).
  resume {ptr, i32} %lp

exit:
  ret void

Maybe we can add an assert to convertConstantExpr checking that the queried constant is not in the map (!valueMapping.count(constant) to avoid this kind of issues? Right now we're only calling this function from convertGlobal (after clearing the map), convertValue and convertMedatadaValue (after checking it is not present in the map), so I'm assuming this shouldn't break anything. WDYT?

[gysit]

Maybe we can add an assert to convertConstantExpr checking that the queried constant is not in the map (!valueMapping.count(constant) to avoid this kind of issues?

I think you are right here! The insertion point should only be updated if the constant does not have a mapping. Updating it may lead to an insertion point that does not point to the end of the constant section anymore. If then further constants are added things may break!

I will prepare a revision for it. Using convertValue solves the problem but it still makes sense to make convertConstantExpr more robust IMO.

Thanks for the explanation.

[gysit]

You are right it is indeed only called from safe places. I ended up adding an assertion as a reminder if things change in the future https://reviews.llvm.org/D149253.

mlir/lib/Target/LLVMIR/ModuleImport.cpp

if (inst->getOpcode() == llvm::Instruction::Call) {
     return success();
   }
   if (inst->getOpcode() == llvm::Instruction::LandingPad) {
     auto *lpInst = cast<llvm::LandingPadInst>(inst);
 
     SmallVector<Value> operands;
     operands.reserve(lpInst->getNumClauses());
     for (auto i : llvm::seq<unsigned>(0, lpInst->getNumClauses())) {
-      FailureOr<Value> operand = convertConstantExpr(lpInst->getClause(i));
+      FailureOr<Value> operand = convertValue(lpInst->getClause(i));
       if (failed(operand))
         return failure();
       operands.push_back(*operand);
     }
 
     Type type = convertType(lpInst->getType());
     auto lpOp =
         builder.create<LandingpadOp>(loc, type, lpInst->isCleanup(), operands);

mlir/test/Target/LLVMIR/Import/exception.ll

bb0:
   ret i32 %e
 ; CHECK: ^[[bb5]]:
 bb1:
   ; CHECK: %[[lp:.*]] = llvm.landingpad cleanup : i32
   %resume = landingpad i32 cleanup
   ; CHECK: llvm.resume %[[lp]] : i32
   resume i32 %resume
 }
+
+declare void @f0(ptr)
+declare void @f1(i32)
+declare void @f2({ptr, i32})
+
+; CHECK-LABEL: @landingpad_dominance
+define void @landingpad_dominance() personality ptr @__gxx_personality_v0 {
+entry:
+  ; CHECK:    %[[null:.*]] = llvm.mlir.null : !llvm.ptr
+  ; CHECK:    %[[c1:.*]] = llvm.mlir.constant(0 : i32) : i32
+  ; CHECK:    %[[undef:.*]] = llvm.mlir.undef : !llvm.struct<(ptr, i32)>
+  ; CHECK:    %[[tmpstruct:.*]] = llvm.insertvalue %[[null]], %[[undef]][0] : !llvm.struct<(ptr, i32)>
+  ; CHECK:    %[[struct:.*]] = llvm.insertvalue %[[c1]], %[[tmpstruct]][1] : !llvm.struct<(ptr, i32)>
+  ; CHECK:    llvm.call @f0(%[[null]]) : (!llvm.ptr) -> ()
+  call void @f0(ptr null)
+  ; CHECK:    llvm.call @f1(%[[c1]]) : (i32) -> ()
+  call void @f1(i32 0)
+  ; CHECK:    llvm.invoke @f0(%[[null]]) to ^[[block2:.*]] unwind ^[[block1:.*]] : (!llvm.ptr) -> ()
+  invoke void @f0(ptr null)
+      to label %exit unwind label %catch
+
+; CHECK:  ^[[block1]]:
+catch:
+  ; CHECK:    %[[lp:.*]] = llvm.landingpad (catch %[[null]] : !llvm.ptr) : !llvm.struct<(ptr, i32)>
+  %lp = landingpad { ptr, i32 } catch ptr null
+  ; CHECK:    llvm.call @f2(%[[struct]]) : (!llvm.struct<(ptr, i32)>) -> ()
+  call void @f2({ptr, i32} {ptr null, i32 0})
+  ; CHECK:    llvm.resume %[[lp]] : !llvm.struct<(ptr, i32)>
+  resume {ptr, i32} %lp
+
+; CHECK:  ^[[block2]]:
+exit:
+  ; CHECK:    llvm.return
+  ret void
+}