[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 ↗

(On Diff #52488)

Why did you remove this?

[• ubsan]

tools/llvm-c-test/echo.cpp

295 ↗

(On Diff #52488)

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 ↗

(On Diff #52488)

Are these value stable ?

tools/llvm-c-test/echo.cpp

295 ↗

(On Diff #52488)

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 ↗

(On Diff #52488)

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

[• ubsan]

include/llvm-c/Core.h

252 ↗

(On Diff #52488)

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 ↗

(On Diff #52694)

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

297 ↗

(On Diff #52694)

Keep the comment please.

[• ubsan]

Coalesce the checks.

llvm/trunk/include/llvm-c/Core.h

typedef enum {
   LLVMColdCallConv        = 9,
   LLVMWebKitJSCallConv    = 12,
   LLVMAnyRegCallConv      = 13,
   LLVMX86StdcallCallConv  = 64,
   LLVMX86FastcallCallConv = 65
 } LLVMCallConv;
 
 typedef enum {
+  LLVMArgumentValueKind,
+  LLVMBasicBlockValueKind,
+  LLVMMemoryUseValueKind,
+  LLVMMemoryDefValueKind,
+  LLVMMemoryPhiValueKind,
+
+  LLVMFunctionValueKind,
+  LLVMGlobalAliasValueKind,
+  LLVMGlobalVariableValueKind,
+  LLVMBlockAddressValueKind,
+  LLVMConstantExprValueKind,
+  LLVMConstantArrayValueKind,
+  LLVMConstantStructValueKind,
+  LLVMConstantVectorValueKind,
+
+  LLVMUndefValueValueKind,
+  LLVMConstantAggregateZeroValueKind,
+  LLVMConstantDataArrayValueKind,
+  LLVMConstantDataVectorValueKind,
+  LLVMConstantIntValueKind,
+  LLVMConstantFPValueKind,
+  LLVMConstantPointerNullValueKind,
+  LLVMConstantTokenNoneValueKind,
+
+  LLVMMetadataAsValueValueKind,
+  LLVMInlineAsmValueKind,
+
+  LLVMInstructionValueKind,
+} LLVMValueKind;
+
+typedef enum {
   LLVMIntEQ = 32, /**< equal */
   LLVMIntNE,      /**< not equal */
   LLVMIntUGT,     /**< unsigned greater than */
   LLVMIntUGE,     /**< unsigned greater or equal */
   LLVMIntULT,     /**< unsigned less than */
   LLVMIntULE,     /**< unsigned less or equal */
   LLVMIntSGT,     /**< signed greater than */
   LLVMIntSGE,     /**< signed greater or equal */

 /**
  * Obtain the type of a value.
  *
  * @see llvm::Value::getType()
  */
 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()
  */
 const char *LLVMGetValueName(LLVMValueRef Val);
 
 /**
  * Set the string name of a value.

llvm/trunk/lib/IR/Core.cpp

 /*===-- Operations on values ----------------------------------------------===*/
 
 /*--.. Operations on all values ............................................--*/
 
 LLVMTypeRef LLVMTypeOf(LLVMValueRef Val) {
   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();
 }
 
 void LLVMSetValueName(LLVMValueRef Val, const char *Name) {
   unwrap(Val)->setName(Name);
 }
 

llvm/trunk/tools/llvm-c-test/echo.cpp

static ValueMap clone_params(LLVMValueRef Src, LLVMValueRef Dst) {
 
   if (Count != 0)
     report_fatal_error("Parameter count does not match iteration");
 
   return VMap;
 }
 
 LLVMValueRef clone_constant(LLVMValueRef Cst, LLVMModuleRef M) {
+  LLVMValueRef Ret;
   if (!LLVMIsAConstant(Cst))
     report_fatal_error("Expected a constant");
 
   // Maybe it is a symbol
   if (LLVMIsAGlobalValue(Cst)) {
     const char *Name = LLVMGetValueName(Cst);
 
     // Try function
     if (LLVMIsAFunction(Cst)) {
-      LLVMValueRef Dst = LLVMGetNamedFunction(M, Name);
-      if (Dst)
-        return Dst;
+      if (LLVMGetValueKind(Cst) != LLVMFunctionValueKind)
+        report_fatal_error("LLVMGetValueKind returned incorrect type");
+      Ret = LLVMGetNamedFunction(M, Name);
+      if (!Ret)
         report_fatal_error("Could not find function");
-    }
-
     // Try global variable
-    if (LLVMIsAGlobalVariable(Cst)) {
-      LLVMValueRef Dst = LLVMGetNamedGlobal(M, Name);
-      if (Dst)
-        return Dst;
+    } else if (LLVMIsAGlobalVariable(Cst)) {
+      if (LLVMGetValueKind(Cst) != LLVMGlobalVariableValueKind)
+        report_fatal_error("LLVMGetValueKind returned incorrect type");
+      Ret = LLVMGetNamedGlobal(M, Name);
+      if (!Ret)
         report_fatal_error("Could not find function");
-    }
-
+    } else {
       fprintf(stderr, "Could not find @%s\n", Name);
       exit(-1);
     }
-
   // Try integer literal
-  if (LLVMIsAConstantInt(Cst))
-    return LLVMConstInt(TypeCloner(M).Clone(Cst),
-                        LLVMConstIntGetZExtValue(Cst), false);
-
+  } else if (LLVMIsAConstantInt(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantIntValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
+    Ret = LLVMConstInt(TypeCloner(M).Clone(Cst), LLVMConstIntGetZExtValue(Cst),
+                       false);
   // Try zeroinitializer
-  if (LLVMIsAConstantAggregateZero(Cst))
-    return LLVMConstNull(TypeCloner(M).Clone(Cst));
-
+  } else if (LLVMIsAConstantAggregateZero(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMConstantAggregateZeroValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
+    Ret = LLVMConstNull(TypeCloner(M).Clone(Cst));
   // Try constant array
-  if (LLVMIsAConstantArray(Cst)) {
+  } else 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;
     for (unsigned i = 0; i < EltCount; i++)
       Elts.push_back(clone_constant(LLVMGetOperand(Cst, i), M));
-    return LLVMConstArray(LLVMGetElementType(Ty), Elts.data(), EltCount);
-  }
-
+    Ret = LLVMConstArray(LLVMGetElementType(Ty), Elts.data(), EltCount);
   // Try contant data array
-  if (LLVMIsAConstantDataArray(Cst)) {
+  } else 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;
     for (unsigned i = 0; i < EltCount; i++)
       Elts.push_back(clone_constant(LLVMGetElementAsConstant(Cst, i), M));
-    return LLVMConstArray(LLVMGetElementType(Ty), Elts.data(), EltCount);
-  }
-
+    Ret = LLVMConstArray(LLVMGetElementType(Ty), Elts.data(), EltCount);
   // Try constant struct
-  if (LLVMIsAConstantStruct(Cst)) {
+  } else 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;
     for (unsigned i = 0; i < EltCount; i++)
       Elts.push_back(clone_constant(LLVMGetOperand(Cst, i), M));
     if (LLVMGetStructName(Ty))
-      return LLVMConstNamedStruct(Ty, Elts.data(), EltCount);
-    return LLVMConstStructInContext(LLVMGetModuleContext(M), Elts.data(),
+      Ret = LLVMConstNamedStruct(Ty, Elts.data(), EltCount);
+    else
+      Ret = LLVMConstStructInContext(LLVMGetModuleContext(M), Elts.data(),
                                      EltCount, LLVMIsPackedStruct(Ty));
-  }
-
   // Try undef
-  if (LLVMIsUndef(Cst))
-    return LLVMGetUndef(TypeCloner(M).Clone(Cst));
-
+  } else if (LLVMIsUndef(Cst)) {
+    if (LLVMGetValueKind(Cst) != LLVMUndefValueValueKind)
+      report_fatal_error("LLVMGetValueKind returned incorrect type");
+    Ret = LLVMGetUndef(TypeCloner(M).Clone(Cst));
   // Try float literal
-  if (LLVMIsAConstantFP(Cst))
+  } else 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))
+  } else if (!LLVMIsAConstantExpr(Cst)) {
     report_fatal_error("Expected a constant expression");
-
-  // At this point, it must be a constant expression
+  // At this point, it must be a constant expression, and thus, an opcode
+  } else {
     LLVMOpcode Op = LLVMGetConstOpcode(Cst);
     switch(Op) {
       case LLVMBitCast:
         return LLVMConstBitCast(clone_constant(LLVMGetOperand(Cst, 0), M),
                                 TypeCloner(M).Clone(Cst));
       default:
         fprintf(stderr, "%d is not a supported opcode\n", Op);
         exit(-1);
     }
   }
+  if (LLVMGetValueKind(Ret) != LLVMGetValueKind(Cst)) {
+    report_fatal_error(
+        "The ValueKind of Ret is not equal to the ValueKind of Cst");
+  }
+  return Ret;
+}
 
 struct FunCloner {
   LLVMValueRef Fun;
   LLVMModuleRef M;
 
   ValueMap VMap;
   BasicBlockMap BBMap;
 
LLVMValueRef CloneValue(LLVMValueRef Src) {
     // Function argument should always be in the map already.
     auto i = VMap.find(Src);
     if (i != VMap.end())
       return i->second;
 
     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));
     LLVMPositionBuilderAtEnd(Builder, BB);
     auto Dst = CloneInstruction(Src, Builder);
     LLVMDisposeBuilder(Builder);
     return Dst;
   }
 
   LLVMValueRef CloneInstruction(LLVMValueRef Src, LLVMBuilderRef Builder) {
     const char *Name = LLVMGetValueName(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);
       if (i != VMap.end()) {
         // If we have a hit, it means we already generated the instruction
         // as a dependancy to somethign else. We need to make sure
         // it is ordered properly.
         auto I = i->second;