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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 7 inline comments as done.
kristof.beyls added inline comments.


================
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");
----------------
ab wrote:
> Not a big deal, but maybe use getMask() in the asserts too?  (and make the message more generic, like 'AddressSpace too large')
I intended to do that in the previous iteration of the patch, but forgot...
I decided to move this assert to the maskAndShift method, so that I didn't unnecessarily have to copy paste the check.


================
Comment at: include/llvm/Support/LowLevelTypeImpl.h:194
+      if (!IsPointer)
+        RawData = maskAndShift(SizeInBits, 0xFFFFFFFF, 0);
+      else
----------------
ab wrote:
> 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!
Yeah, I agree the [2] makes it a bit weird.
I introduced a "typedef int BitFieldInfo[2];", so the [2] doesn't appear in lots of places to make the code read a bit more fluently.
I also renamed the BitFieldInfo variables to end with name "FieldInfo" rather than "Field", as they're not fields, but info about fields.


================
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:
> 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?
Indeed, that was what was needed. Thanks for the pointer!


https://reviews.llvm.org/D31934