[PATCH] D31934: [GlobalISel] Support vector-of-pointers in LLT

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

You're right, this is probably the best path forward.  A few nits inline



================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:45
     assert(SizeInBits > 0 && "invalid scalar size");
-    return LLT{Scalar, 1, SizeInBits};
+    return LLT{false, false, 0, SizeInBits, 0};
   }
----------------
Maybe:
  return LLT{/*isPointer=*/false, /*isVector=*/false, /*NumElements=*/0, SizeInBits, /*AddressSpace=*/0};

Makes it a bit easier to read IMO


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:165
+  /// Pointer (isPointer == 1 && isVector == 0):
+  ///   SizeInBits: 32;
+  ///   AddressSpace: 23;
----------------
Maybe make this 16 bits to match the vector-of-pointer case?  I'm a bit worried about it being different.


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:181
+
+  static uint64_t maskAndShift(uint64_t val, uint64_t mask, uint8_t shift) {
+    return (val & mask) << shift;
----------------
val -> Val (here and elsewhere)


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:194
+      if (!IsPointer)
+        RawData = maskAndShift(SizeInBits, 0xFFFFFFFF, 0);
+      else
----------------
Maybe add some static consts for the masks?  You could then merge those with the comment explaining the layout.


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:213
+  unsigned getScalarSizeInBits() const {
+    assert(RawData != 0);
+    if (!IsVector) {
----------------
Perhaps:
    && "Invalid type"


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:489
     LLT Ty = MRI.getType(MO.getReg());
-    OpSize[Idx] = Ty.getSizeInBits();
+    OpSize[Idx] = Ty.isValid() ? Ty.getSizeInBits() : 0;
 
----------------
Huh, how does an instruction survive this far with an invalid type?


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll:151
+; AArch64 was asserting when using vectors of pointers, which cannot be
+; represented in LLT at the moment.
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to legalize instruction: %vreg0<def>(p0) = G_EXTRACT_VECTOR_ELT %vreg1, %vreg2; (in function: vector_of_pointers_extractelement)
----------------
Immediately stale comment? ;)


https://reviews.llvm.org/D31934