[llvm-c] Improve IR Introspection: Add ValueKind

[• ubsan]

Summary

Bringing D2176 to modernity

Adds:

• LLVMValueKind
• LLVMGetValueKind

[• ubsan]

I'm sorry, learning the tools :)

[• ubsan]

listening to whitequark :P

[• ubsan]

Add tests, fix that switch in LLVMGetValueKind

[whitequark]

tools/llvm-c-test/echo.cpp

295

Why did you remove this?

[• ubsan]

tools/llvm-c-test/echo.cpp

295

The exact same test is also at the top of the function.

[deadalnix]

Could you use arc next time to submit a patch ? It is very hard to see if there is missing checks in the test.

Overall, if the stability of these values cannot be guaranteed then that is an issue and a mechanism to retrieve the ID, either from a string or from a C enum with guaranteed values.

More generally, I'm not sure how much value does this bring to the table as it seems fairly redundant with the LLVMIsAXXXX mechanism. It can be argued that it create a few extra tests, but using the C API means wrapping and unwrapping all the time, and doing these check in the process, so I'm not sure it is worth it.

include/llvm-c/Core.h

252

Are these value stable ?

tools/llvm-c-test/echo.cpp

295

No it is not.

[whitequark]

The reason the table exists is for bindings. Without LLVMGetValueKind(), a binding that wishes to introspect an instruction has to call the multitude of LLVMIsA* functions in sequence, which is:

1. Slow (many bindings use libffi)
2. Usually cannot be inlined (see 1)
3. Fragile and hostile to upgrades of LLVM, as there isn't even an easy way to figure out the type of the value that broke the check

[deadalnix]

If you guys think exposing this id is a good idea I won't oppose it. If I understand correctly, you want to avoid calling into LLVM too much in case the call itself is expensive ? It seems to me that the C API doesn't do a very good job at reducing the amount of calls, for instance various instructions cannot be built without multiple calls (among the worse offenders, landingpads). There is probably lower hanging fruits in that tree.

Anyway, as long as the enum value are stable, I'm won't oppose this. I just wanted to make sure you understands that goign froward with this means having an enum to maintain like forever.

[• ubsan]

tools/llvm-c-test/echo.cpp

295

Ah. I misread. I will add it back in.

[• ubsan]

include/llvm-c/Core.h

252

No... hmm, you're right. We could still do this, but we would not be able to use Value.def, because we have to make LLVMInstructionValueKind the last.

[• ubsan]

Fix deadalnix's problems with the revision.

[• ubsan]

So how this would work is; any additional values would be added after LLVMInstructionValueKind, and if any values are removed, they would be removed in this enum, and the values would have integer values assigned.

[whitequark]

LGTM.

[deadalnix]

Ok one last thing. In the test, that'd be great to check that a cloned value has the same value kind as the "clonee" in the test.

[• ubsan]

Can do.

[• ubsan]

Add checks for the Ret value as well

[deadalnix]

tools/llvm-c-test/echo.cpp

230–231

Instead of reproducing this pattern of over the place, I think it would make sense to factor it out.

297

Keep the comment please.

[• ubsan]

Coalesce the checks.

include/llvm-c/Core.h

 } LLVMCallConv;
 
 typedef enum {
+#define HANDLE_VALUE(Name) LLVM##Name##ValueKind,
+#include "llvm/IR/Value.def"
+} LLVMValueKind;
+
+typedef enum {
   LLVMIntEQ = 32, /**< equal */
   LLVMIntNE,      /**< not equal */
   LLVMIntUGT,     /**< unsigned greater than */

 LLVMTypeRef LLVMTypeOf(LLVMValueRef Val);
 
 /**
+ * Obtain the enumerated type of a Value instance.
+ *
+ * @see llvm::Value::getValueID()
+ */
+LLVMValueKind LLVMGetValueKind(LLVMValueRef Val);
+
+/**
  * Obtain the string name of a value.
  *
  * @see llvm::Value::getName()

lib/IR/Core.cpp

   return wrap(unwrap(Val)->getType());
 }
 
+LLVMValueKind LLVMGetValueKind(LLVMValueRef Val) {
+    switch(unwrap(Val)->getValueID()) {
+#define HANDLE_VALUE(Name) \
+  case Value::Name##Val: \
+    return LLVM##Name##ValueKind;
+#include "llvm/IR/Value.def"
+  default:
+    return LLVMInstructionValueKind;
+  }
+}
+
 const char *LLVMGetValueName(LLVMValueRef Val) {
   return unwrap(Val)->getName().data();
 }

tools/llvm-c-test/echo.cpp

 
     // Try function
     if (LLVMIsAFunction(Cst)) {
+      if (LLVMGetValueKind(Cst) != LLVMFunctionValueKind)
+        report_fatal_error("LLVMGetValueKind returned incorrect type");
       LLVMValueRef Dst = LLVMGetNamedFunction(M, Name);
       if (Dst)
         return Dst;

 
     // Try global variable
     if (LLVMIsAGlobalVariable(Cst)) {
+      if (LLVMGetValueKind(Cst) != LLVMGlobalVariableValueKind)
+        report_fatal_error("LLVMGetValueKind returned incorrect type");
       LLVMValueRef Dst = LLVMGetNamedGlobal(M, Name);
       if (Dst)
         return Dst;

   }
 
   // Try integer literal
-  if (LLVMIsAConstantInt(Cst))
+  if (LLVMIsAConstantInt(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantIntValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     return LLVMConstInt(TypeCloner(M).Clone(Cst),
                         LLVMConstIntGetZExtValue(Cst), false);
+  }
 
   // Try zeroinitializer
-  if (LLVMIsAConstantAggregateZero(Cst))
+  if (LLVMIsAConstantAggregateZero(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantAggregateZeroValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     return LLVMConstNull(TypeCloner(M).Clone(Cst));
+  }
 
   // Try constant array
   if (LLVMIsAConstantArray(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantArrayValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     LLVMTypeRef Ty = TypeCloner(M).Clone(Cst);
     unsigned EltCount = LLVMGetArrayLength(Ty);
     SmallVector<LLVMValueRef, 8> Elts;

 
   // Try contant data array
   if (LLVMIsAConstantDataArray(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantDataArrayValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     LLVMTypeRef Ty = TypeCloner(M).Clone(Cst);
     unsigned EltCount = LLVMGetArrayLength(Ty);
     SmallVector<LLVMValueRef, 8> Elts;

 
   // Try constant struct
   if (LLVMIsAConstantStruct(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantStructValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     LLVMTypeRef Ty = TypeCloner(M).Clone(Cst);
     unsigned EltCount = LLVMCountStructElementTypes(Ty);
     SmallVector<LLVMValueRef, 8> Elts;

   }
 
   // Try undef
-  if (LLVMIsUndef(Cst))
+  if (LLVMIsUndef(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMUndefValueValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     return LLVMGetUndef(TypeCloner(M).Clone(Cst));
+  }
 
   // Try float literal
-  if (LLVMIsAConstantFP(Cst))
+  if (LLVMIsAConstantFP(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantFPValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
     report_fatal_error("ConstantFP is not supported");
+  }
 
-  // This kind of constant is not supported
-  if (!LLVMIsAConstantExpr(Cst))
-    report_fatal_error("Expected a constant expression");
-
-  // At this point, it must be a constant expression
+  // At this point, we've exhausted all supported options
   LLVMOpcode Op = LLVMGetConstOpcode(Cst);
   switch(Op) {
     case LLVMBitCast:

     if (!LLVMIsAInstruction(Src))
       report_fatal_error("Expected an instruction");
 
+    if (LLVMGetValueKind(Src) != LLVMInstructionValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
+
     auto Ctx = LLVMGetModuleContext(M);
     auto Builder = LLVMCreateBuilderInContext(Ctx);
     auto BB = DeclareBB(LLVMGetInstructionParent(Src));

     if (!LLVMIsAInstruction(Src))
       report_fatal_error("Expected an instruction");
 
+    if (LLVMGetValueKind(Src) != LLVMInstructionValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
+
     // Check if this is something we already computed.
     {
       auto i = VMap.find(Src);