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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:194
+      if (!IsPointer)
+        RawData = maskAndShift(SizeInBits, 0xFFFFFFFF, 0);
+      else
----------------
kristof.beyls wrote:
> ab wrote:
> > Maybe add some static consts for the masks?  You could then merge those with the comment explaining the layout.
> I experimented with a number of different ways to improve this.
> The main issue here is that there isn't a way that is both portable and simple to have bitfield unions with full control on bitfield layout in C++ (at least not that I know off).
> Apart from resorting to try and create a "union bitfield" ADT, I think either the original code, or the new code here are the most reasonable ways to encode this. I think these *Field descriptors in the new patch is slightly better than the original code I wrote with hard-coded masks etc., but only just.
The [2] are a bit weird, but it does make the bit magic more readable, thanks!


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:226
+           "invalid number of vector elements");
+    assert(AddressSpace <= 0x7FFFFF && "AddressSpace must fit in 23 bits");
+    assert(NumElements <= 0xFFFF && "NumElements must fit in 16 bits");
----------------
Not a big deal, but maybe use getMask() in the asserts too?  (and make the message more generic, like 'AddressSpace too large')


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:540
   for (unsigned Idx = 0; Idx < NumOperands; ++Idx) {
     if (MI.getOperand(Idx).isReg()) {
       auto Mapping = getValueMapping(OpRegBankIdx[Idx], OpSize[Idx]);
----------------
This would probably also need to check that the vreg isn't 0.


================
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;
 
----------------
kristof.beyls wrote:
> ab wrote:
> > Huh, how does an instruction survive this far with an invalid type?
> I see the following test failing here if this doesn't get changed: CodeGen/AArch64/GlobalISel/regbankselect-dbg-value.mir.
> Before this patch, LLT::getSizeInBits() didn't assert on an invalid type, so the invalid type from that test case always seems to have ended up here.
> IIRC, it was the following IR line that triggered this, more specifically the operand "debug-use _".
> 
> ```
> DBG_VALUE debug-use %0(s32), debug-use _, !7, !9, debug-location !10
> 
> ```
>  I didn't investigate further.
Ah, then I suspect this should ignore %noreg:

  if (!MO.getReg())
    continue;

before trying to get the vreg type.  Does that help?


https://reviews.llvm.org/D31934