[PATCH] D20387: [AArch64] Generate a BFI/BFXIL from 'or (and X, MaskImm), OrImm' iff the value being inserted only sets known zero bits.

[Geoff Berry via llvm-commits]

gberry added inline comments.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2019
@@ +2018,3 @@
+  // point if we want to use this value.
+  uint64_t NonZeroBits = (~KnownZero).getZExtValue();
+
----------------
NotKnownZero might be a clearer name for this, or just get rid of it and use ~KnownZeroBits instead.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2023
@@ +2022,3 @@
+  // and the bits to be inserted must be a shifted mask.
+  if ((OrImm & NonZeroBits) != 0 || !isShiftedMask(~NonZeroBits, VT))
+    return false;
----------------
I think this is too conservative.  It should be okay to have bits that are 1 in the OrImm that are not KnownZero, since you know they will be 1 in the final result.  E.g. (or (and x, 0xfffffff0), 0xf5).  Essentially the known bits in the final result are KnownZero | OrImm.

Actually, maybe none of the above matters since the IR seems to be canonicalized in this case to widen the and mask to cover all of the 1 bits in the or mask as well.

================
Comment at: test/CodeGen/AArch64/bitfield-insert.ll:381
@@ -380,1 +380,3 @@
 }
+
+; CHECK-LABEL: @test1
----------------
You might want to change the hard-coded x8/w8 to a regex in these tests to make them a little less brittle w.r.t. future changes.

================
Comment at: test/CodeGen/AArch64/bitfield-insert.ll:449
@@ +448,3 @@
+
+; BFIs that requires more instructions to materialize the constant as compared
+; to the original ORR are not okay.  In this case we would be replacing the
----------------
requires -> require


http://reviews.llvm.org/D20387