[PATCH] D65052: [ARM] MVE predicate register support

[Dave Green via Phabricator via llvm-commits]

dmgreen marked 7 inline comments as done.
dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:7398
+  SDValue AllOnes =
+      DAG.getTargetConstant(ARM_AM::createNEONModImm(0xe, 0xff), dl, MVT::i32);
+  AllOnes = DAG.getNode(ARMISD::VMOVIMM, dl, MVT::v16i8, AllOnes);
----------------
SjoerdMeijer wrote:
> > I can rename it easily enough. createVModImm sound OK?
> 
> Yep, sounds good
It ended up as createVMOVModImm in rL366790.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:7666
+  unsigned Lane = cast<ConstantSDNode>(Op.getOperand(2))->getZExtValue();
+  unsigned LaneWidth = 16 / VecVT.getVectorNumElements();
+  unsigned Mask = ((1 << LaneWidth) - 1) << Lane * LaneWidth;
----------------
SjoerdMeijer wrote:
> Perhaps better to hide magic constant 16 in a MVE subtarget function or something like that?
I changed this around a little to use the scalar size.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-pred-build-var.ll:188
+; CHECK-NEXT:    rsbs r0, r0, #0
+; CHECK-NEXT:    bfi r2, r0, #0, #1
+; CHECK-NEXT:    bfi r2, r0, #1, #1
----------------
samparker wrote:
> Why aren't these just one: bfi r2, r0, #0, #16 or uxth r2, r0?
Thats "FIXME: Handle splats of the same value better." Maybe just a conditional mov of -1 (or something)? It's on the list of things to do.


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

https://reviews.llvm.org/D65052