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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 6 inline comments as done.
kristof.beyls added a comment.

Thanks very much for the review Ahmed! I think I took all feedback into account in the new version of the patch. Please have another look.



================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:194
+      if (!IsPointer)
+        RawData = maskAndShift(SizeInBits, 0xFFFFFFFF, 0);
+      else
----------------
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.


================
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;
 
----------------
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.


https://reviews.llvm.org/D31934