Teach TableGen Intrin Emitter to handle LLVMPointerType<llvm_any_ty>

[erichkeane]

Summary

r363233 rewrote a bunch of the Intrin Emitter code, however the new
function to update the arg codes did not properly consider a pointer to
an any. This patch adds that logic.

[erichkeane]

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

[greened]

Unfortunately, I've never worked in the IntrinsicEmitter so I can't really comment on the correctness of the patch. I will make some inline comments on non-correctness things.

[greened]

llvm/test/TableGen/intrinsic-pointer-to-any.td

2

Could we have a comment here explaining what this is testing and what went wrong before this patch?

23

Do we need this and the below parameters for the test? AFAICT they are never passed.

26

Are the members here expected to exist by IntrinsicEmitter? If so, defaults could be set directly rather than using default arguments that are never passed. If not, we could just delete them.

[erichkeane]

llvm/test/TableGen/intrinsic-pointer-to-any.td

23

They're not needed, though I was trying to just emulate what the actual definition of Intrinsic was. I'll change it.

26

They all seem required. I can do that.

[sdesmalen]

In D63507#1551968, @erichkeane wrote:

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Thanks, I definitely want to take a look but I’m currently traveling without access to my laptop, so I won’t be able to properly review it until Wednesday.

[greened]

llvm/utils/TableGen/IntrinsicEmitter.cpp

375

Since I don't know this code I can't say whether it's correct, but it seems odd to me that this iPTR case is needed to handle ptr-to-any when iPTRAny is a case right below. Does iPTRAny not mean "ptr-to-any?" At the very least a comment would be helpful explaining what is going on here and why we need a separate iPTR case to handle ptr-to-any.

[erichkeane]

In D63507#1552849, @sdesmalen wrote:

In D63507#1551968, @erichkeane wrote:

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Thanks, I definitely want to take a look but I’m currently traveling without access to my laptop, so I won’t be able to properly review it until Wednesday.

Great, thanks!

llvm/utils/TableGen/IntrinsicEmitter.cpp

375

iPTRAny is actually "Any Pointer", not "Pointer to Any". I'm not terribly sure why it makes a difference, but it does.

[hfinkel]

llvm/utils/TableGen/IntrinsicEmitter.cpp

375

I believe that iPTR is a pointer in some specified address space (0, by default), and iPTRAny is a pointer in any address space.

[hfinkel]

LGTM

[sdesmalen]

The patch already landed, but also adding my LGTM. It seems like a case that I missed in my previous patch, thanks for fixing!

llvm/test/TableGen/intrinsic-pointer-to-any.td

+// RUN: llvm-tblgen -gen-intrinsic-impl %s | FileCheck %s
+
+// This test is validating that it an Intrinsic with an LLVMPointerType to
+// llvm_any_ty still properly work after r363233. That patch rewrote the
+// substitution handling code in the Intrinsic Emitter, and didn't consider this
+// case, so TableGen would hit an assertion in EncodeFixedType that was checking
+// to ensure that the substitution being processed was correctly replaced.
+
+class IntrinsicProperty;
+class SDNodeProperty;
+
+class ValueType<int size, int value> {
+  string Namespace = "MVT";
+  int Size = size;
+  int Value = value;
+}
+
+def iPTR   : ValueType<0  , 254>;
+def Any    : ValueType<0  , 255>;
+
+class LLVMType<ValueType vt> {
+  ValueType VT = vt;
+  int isAny = 0;
+}
+
+
+class Intrinsic<list<LLVMType> ret_types> {
+  string LLVMName = "";
+  string TargetPrefix = "";   // Set to a prefix for target-specific intrinsics.
+  list<LLVMType> RetTypes = ret_types;
+  list<LLVMType> ParamTypes = [];
+  list<IntrinsicProperty> IntrProperties = [];
+  list<SDNodeProperty> Properties = [];
+  bit isTarget = 0;
+}
+
+class LLVMQualPointerType<LLVMType elty>
+  : LLVMType<iPTR>{
+  LLVMType ElTy = elty;
+  int AddrSpace = 0;
+}
+
+class LLVMPointerType<LLVMType elty>
+  : LLVMQualPointerType<elty>;
+
+let isAny = 1 in {
+  def llvm_any_ty        : LLVMType<Any>;
+}
+def i8 : ValueType<8, 3>;
+def llvm_i8_ty : LLVMType<i8>;
+
+def int_has_ptr_to_any : Intrinsic<[LLVMPointerType<llvm_any_ty>, llvm_i8_ty]>;
+// CHECK: /* 0 */ 21, 14, 15, 0, 2, 0

llvm/utils/TableGen/IntrinsicEmitter.cpp

if (R->isSubClassOf("LLVMVectorOfAnyPointersToElt")) {
       ++NumInserted;
     }
     return;
   }
 
   unsigned Tmp = 0;
   switch (getValueType(R->getValueAsDef("VT"))) {
   default: break;
+  case MVT::iPTR:
+    UpdateArgCodes(R->getValueAsDef("ElTy"), ArgCodes, NumInserted, Mapping);
+    break;
   case MVT::iPTRAny:
     ++Tmp;
     LLVM_FALLTHROUGH;
   case MVT::vAny:
     ++Tmp;
     LLVM_FALLTHROUGH;
   case MVT::fAny:
     ++Tmp;