[ExecutionEngine] Support TargetExtType in Interpreter

[wenju]

Summary

When llvm Interpreter is used to execute bitcode, it shall be able to handle TargetExtType.

[wenju]

kindly ping @jcranmer-intel

[jcranmer-intel]

Sorry for the delay in reviewing this patch.

I'm not an expert in the JIT code, so I'm not completely certain that this code covers all the bases (in particular, it's somewhat unclear to me if this is handling globals correctly).

You should be able to add a test for this code along the lines of test/ExecutionEngine/test-interp-aggregate.ll, that would at least make sure that load/store, global variables, ConstantTargetNone, and poison value initializers work correctly. That would definitely make me more comfortable re the global variable support.

[wenju]

add load/store/constant lit test for changes in ExecutionEngine.cpp

[wenju]

I don't have a test case of global variable with target extension type.

poison value initializers

doe this mean something like "store target("spirv.Event") poison, ptr %event, align 8"?

An alterative approach to this patch is to lower target extension type to its layout type before executing interpreter. I'm proposing this patch as I think it might worth adding natively support of target extension type to interpreter. I don't know if this support will be useful for someone else. There could be cases that target extension type exists in bitcode input of an interpreter.

[jcranmer-intel]

In D148283#4398589, @wenju wrote:

I don't have a test case of global variable with target extension type.

A simple @global = target("spirv.Event") zeroinitializer should suffice, combined with a load or store of that global. The point is to make sure that the interpreter can handle a global variable having a target extension type without crashing.

poison value initializers

doe this mean something like "store target("spirv.Event") poison, ptr %event, align 8"?

Yes.

An alterative approach to this patch is to lower target extension type to its layout type before executing interpreter. I'm proposing this patch as I think it might worth adding natively support of target extension type to interpreter. I don't know if this support will be useful for someone else. There could be cases that target extension type exists in bitcode input of an interpreter.

That's another approach I considered, but I think this approach works a little better. Although you did remind me that there should be a comment somewhere justifying that it is safe to treat the target extension type as its layout type, since the only thing the interpreter is doing is zero-initializing or copying the bits around, and never otherwise inspecting or touching them.

[wenju]

add test of global and poison value, add comment of 'It is safe to treat TargetExtType as its layout type'

[jcranmer-intel]

Thanks, LGTM.

[wenju]

@jcranmer-intel thank you for the review

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
     }
 
     SmallString<256> Msg;
     raw_svector_ostream OS(Msg);
     OS << "ConstantExpr not handled: " << *CE;
     report_fatal_error(OS.str());
   }
 
+  if (auto *TETy = dyn_cast<TargetExtType>(C->getType())) {
+    assert(TETy->hasProperty(TargetExtType::HasZeroInit) && C->isNullValue() &&
+           "TargetExtType only supports null constant value");
+    C = Constant::getNullValue(TETy->getLayoutType());
+  }
+
   // Otherwise, we have a simple constant.
   GenericValue Result;
   switch (C->getType()->getTypeID()) {
   case Type::FloatTyID:
     Result.FloatVal = cast<ConstantFP>(C)->getValueAPF().convertToFloat();
     break;
   case Type::DoubleTyID:
     Result.DoubleVal = cast<ConstantFP>(C)->getValueAPF().convertToDouble();
default:
     report_fatal_error(OS.str());
   }
 
   return Result;
 }
 
 void ExecutionEngine::StoreValueToMemory(const GenericValue &Val,
                                          GenericValue *Ptr, Type *Ty) {
+  // It is safe to treat TargetExtType as its layout type since the underlying
+  // bits are only copied and are not inspected.
+  if (auto *TETy = dyn_cast<TargetExtType>(Ty))
+    Ty = TETy->getLayoutType();
+
   const unsigned StoreBytes = getDataLayout().getTypeStoreSize(Ty);
 
   switch (Ty->getTypeID()) {
   default:
     dbgs() << "Cannot store value of type " << *Ty << "!\n";
     break;
   case Type::IntegerTyID:
     StoreIntToMemory(Val.IntVal, (uint8_t*)Ptr, StoreBytes);
if (sys::IsLittleEndianHost != getDataLayout().isLittleEndian())
     std::reverse((uint8_t*)Ptr, StoreBytes + (uint8_t*)Ptr);
 }
 
 /// FIXME: document
 ///
 void ExecutionEngine::LoadValueFromMemory(GenericValue &Result,
                                           GenericValue *Ptr,
                                           Type *Ty) {
+  if (auto *TETy = dyn_cast<TargetExtType>(Ty))
+    Ty = TETy->getLayoutType();
+
   const unsigned LoadBytes = getDataLayout().getTypeStoreSize(Ty);
 
   switch (Ty->getTypeID()) {
   case Type::IntegerTyID:
     // An APInt with all words initially zero.
     Result.IntVal = APInt(cast<IntegerType>(Ty)->getBitWidth(), 0);
     LoadIntFromMemory(Result.IntVal, (uint8_t*)Ptr, LoadBytes);
     break;

llvm/test/ExecutionEngine/test-interp-target-ext-type.ll

+; RUN: %lli -jit-kind=mcjit -force-interpreter=true %s > /dev/null
+
+; Check interpreter isn't crashing in handling target extension type.
+
+@g = global target("spirv.Event") zeroinitializer, align 8
+
+define i32 @main() {
+  %event = alloca target("spirv.Event"), align 8
+  store target("spirv.Event") zeroinitializer, ptr %event, align 8
+  %e = load target("spirv.Event"), ptr %event, align 8
+
+  store target("spirv.Event") poison, ptr @g, align 8
+  %e2 = load target("spirv.Event"), ptr @g, align 8
+
+  ret i32 0
+}