[PATCH] D14803: [AArch64] Fix a corner case in BitFeild select

[Geoff Berry via llvm-commits]

gberry added inline comments.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2070
@@ -2065,3 +2069,3 @@
       // In that case, we can eliminate the AND
       Dst = OrOpd1->getOperand(0);
     else
----------------
weimingz wrote:
> HI Geoff,
> 
> The NumberOfIgnoredHighBits is used here to check if we can remove the AND.
> The current function
>  static bool isBitfieldInsertOpFromOr(SDNode *N, unsigned &Opc, SDValue &Dst,
>                                      SDValue &Src, unsigned &ImmR,
>                                      unsigned &ImmS, SelectionDAG *CurDAG)
> always expects a BFM op if it returns true.
> 
> I tried putting code like
>  if (NumberOfIgnoredHighBits == 32) {
>       Src = Dst;
>     }
> But it doest make much difference.
> 	mov	 w8, w0
> 	bfi	w8, w8, #8, #24
> 	mov	 w9, w0
> 	bfi	w9, w8, #8, #24
> 	mov	 w8, w0
> 	bfi	w8, w9, #8, #24
> 	bfi	w0, w8, #8, #24
> 	lsl	w0, w0, #8
> 
> 
I was thinking something more like the following diff, which will eliminate a mov and a bfi from the final code.  It seems like if we're already computing the bit-liveness of the 'Or' we should take advantage of it.  I'd like to hear someone else's opinion on whether it is worth the effort (since we clearly haven't seen many cases that would hit this or they would have crashed).

@@ -1974,7 +1974,8 @@ static bool isBitfieldPositioningOp(SelectionDAG *CurDAG, SDValue Op,
 // f = Opc Opd0, Opd1, LSB, MSB ; where Opc is a BFM, LSB = imm, and MSB = imm2
 static bool isBitfieldInsertOpFromOr(SDNode *N, unsigned &Opc, SDValue &Dst,
                                      SDValue &Src, unsigned &ImmR,
-                                     unsigned &ImmS, SelectionDAG *CurDAG) {
+                                     unsigned &ImmS, SelectionDAG *CurDAG,
+                                     const APInt &UsefulBits) {
   assert(N->getOpcode() == ISD::OR && "Expect a OR operation");
 
   // Set Opc
@@ -1988,9 +1989,6 @@ static bool isBitfieldInsertOpFromOr(SDNode *N, unsigned &Opc, SDValue &Dst,
 
   // Because of simplify-demanded-bits in DAGCombine, involved masks may not
   // have the expected shape. Try to undo that.
-  APInt UsefulBits;
-  getUsefulBits(SDValue(N, 0), UsefulBits);
-
   unsigned NumberOfIgnoredLowBits = UsefulBits.countTrailingZeros();
   unsigned NumberOfIgnoredHighBits = UsefulBits.countLeadingZeros();
 
@@ -2083,11 +2081,17 @@ SDNode *AArch64DAGToDAGISel::SelectBitfieldInsertOp(SDNode *N) {
   unsigned Opc;
   unsigned LSB, MSB;
   SDValue Opd0, Opd1;
+  EVT VT = N->getValueType(0);
 
-  if (!isBitfieldInsertOpFromOr(N, Opc, Opd0, Opd1, LSB, MSB, CurDAG))
+  APInt NUsefulBits;
+  getUsefulBits(SDValue(N, 0), NUsefulBits);
+  if (!NUsefulBits)
+    return CurDAG->SelectNodeTo(N, TargetOpcode::IMPLICIT_DEF, VT);
+
+  if (!isBitfieldInsertOpFromOr(N, Opc, Opd0, Opd1, LSB, MSB, CurDAG,
+                                NUsefulBits))
     return nullptr;
 
-  EVT VT = N->getValueType(0);
   SDLoc dl(N);
   SDValue Ops[] = { Opd0,
                     Opd1,


================
Comment at: test/CodeGen/AArch64/bitfield-pr25571.ll:4
@@ +3,3 @@
+target datalayout = "e-m:e-i64:64-i128:128-n8:16:32:64-S128"
+target triple = "aarch64--linux-gnu"
+
----------------
You can get rid of the datalayout and triple lines (just pass the triple via a -mtriple= command line flag on the run line).  You may also want to consider adding this test to another pre-existing test file (perhaps bitfield-insert.ll?).

================
Comment at: test/CodeGen/AArch64/bitfield-pr25571.ll:7
@@ +6,3 @@
+define i32 @test(i8 %a, i32 %b) {
+; CHECK-LABEL: test
+; CHECK: bfi
----------------
Add a ':' after test to avoid false matches.

================
Comment at: test/CodeGen/AArch64/bitfield-pr25571.ll:11
@@ +10,3 @@
+; CHECK: bfi
+; CHECK: bfi
+  %conv = zext i8 %a to i32     ;   0 0 0 0 A
----------------
Maybe check for an lsl at the end as well?

================
Comment at: test/CodeGen/AArch64/bitfield-pr25571.ll:12
@@ +11,3 @@
+; CHECK: bfi
+  %conv = zext i8 %a to i32     ;   0 0 0 0 A
+  %shl = shl i32 %b, 8          ;   B2 B1 B0 0
----------------
This comment should be: 0 0 0 A


http://reviews.llvm.org/D14803