[PATCH] D67085: [ARM] Fix loads and stores for v4i1 and v8i1

Mon Sep 9 05:40:38 PDT 2019

dmgreen marked an inline comment as done.
dmgreen added a comment.

> One question though. Loading and story and story data.....do we need to worry about LE and BE here?

I believe that this is OK. At least for these two we are only storing a single byte, so it shouldn't be an issue.

It does bring up the issue of whether the v16i1 is correct. It looks like the instruction is loading/storing 32bits, so might not be really right to use if we should only be storing 16bits, even in LE. And for BE would be putting the bits in the wrong place. Let me try and adjust that one too.

================
Comment at: llvm/test/CodeGen/Thumb2/mve-pred-bitcast.ll:23
 ; CHECK-NEXT:    vmov.i32 q1, #0x0
-; CHECK-NEXT:    vldr p0, [r0]
+; CHECK-NEXT:    vcmp.i32 ne, q2, zr
 ; CHECK-NEXT:    vpsel q0, q0, q1
----------------
samparker wrote:
> I'm missing something here... from my understanding:
> - Select 16 bytes, taken from q2 (0xff) and q1(0x0), building a vector predicate mask in q1.
> - Then we take the bottom 4 bytes from q1, the mask, putting each into a 32-bit lane of q2.
> - Then we compare the 32-bit lanes of q2 against zero.
> - Then we select bytes from q0 (%a) and q1 (zero).
> 
> It's the second point that I don't understand... why do we only access the lower lanes of q1? 
This is converting an i4, as in the bottom 4 bits of r0, into a v4i1, as in the 16 bits of P0. It needs to be sort of "shuffled" or a "signext_inreg'd" to get those bits into the correct places.

So the "step 0" in you list would be copy the bits from r0 to p0 using the msr, and the 4 bits of interest are in bottom 4 bits of p0. The rest is the awkward sign extend.

The alternative is to do this in integer instructions. That may be better, depending on the circumstances. I don't think (hope) this will come up a lot though. We just need to not get it wrong.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67085/new/

https://reviews.llvm.org/D67085