[AArch64][NFC][1/4]Refactor 'isBitfieldPositioningOp' so that DAG nodes with different Opcode are handled with separate helper functions

[mingmingl]

Summary

The rationale to use different helper functions for DAG nodes with different Opcode is to specialize the optimization.

isBitfieldExtractOp shows how specialization based on Opcode could help.

This paves the way for enhancement in D135844: [AArch64][2/4]Regard (shl val, N) as a potential bit-field-positioning op regardless of the number of uses.

[dmgreen]

You can link patches together with the "Edit Related Revision", allowing them to be stacked together to show they are dependant on one another.

These do seems to end up quite different, even if some of the differences here could be reduced.

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2522

If this only has a single use it may be better to inline, so there is only a single variable pointing to Op.

2579–2581

This can go at the start of the function, and use Op.getValueType().

2589

This needn't set Src.

2649–2650

Op.getOpcode()

[mingmingl]

Address comments

1. Call forwarding methods (getOperand, hasOneUse, etc) of class SDValue as much as possible.
2. To minimize displayed diff, reorder functions (declare helpers and define them after 'isBitfieldPositioningOp'); otherwise, newly added helper for AND will be diff'ed against the original 'isBitfieldPositioningOp'

[mingmingl]

In D135843#3857868, @dmgreen wrote:

You can link patches together with the "Edit Related Revision", allowing them to be stacked together to show they are dependant on one another.

These do seems to end up quite different, even if some of the differences here could be reduced.

Done. Thanks for pointing this out!

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2522

Done by calling the forwarding method of SDValue class as much as possible (here and below)

[dmgreen]

Thanks. I agree that with the other patches this sounds like a sensible thing to split out. LGTM

[mingmingl]

Thanks! Going to commit.

[qiongsiwu1]

This patch seems to be causing build failures when a build is configured with -Werror due to the warnings (e.g. https://lab.llvm.org/buildbot/#/builders/57/builds/22957). May I get your help @mingmingl for some advices/fixes to get the build going? Thank you!

/home/buildbots/clang.11.0.0/bin/clang++ --gcc-toolchain=/opt/rh/devtoolset-7/root/usr  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Target/AArch64 -I/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/lib/Target/AArch64 -Iinclude -I/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fPIC    -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/Target/AArch64/CMakeFiles/LLVMAArch64CodeGen.dir/AArch64ISelDAGToDAG.cpp.o -MF lib/Target/AArch64/CMakeFiles/LLVMAArch64CodeGen.dir/AArch64ISelDAGToDAG.cpp.o.d -o lib/Target/AArch64/CMakeFiles/LLVMAArch64CodeGen.dir/AArch64ISelDAGToDAG.cpp.o -c /home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2596:14: error: comparison of integers of different signs: 'uint64_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
  if (ShlImm != DstLSB && !BiggerPattern)
      ~~~~~~ ^  ~~~~~~
/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2661:14: error: comparison of integers of different signs: 'int' and 'uint64_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
  if (DstLSB != ShlImm && !BiggerPattern)
      ~~~~~~ ^  ~~~~~~
2 errors generated.

[qiongsiwu1]

Ah I just saw 5d3139aef113881fff9430c96069faf96630a5d8. Thanks @MaskRay !

[mingmingl]

In D135843#3862662, @qiongsiwu1 wrote:

Ah I just saw 5d3139aef113881fff9430c96069faf96630a5d8. Thanks @MaskRay !

Ahhh i missed the message completely (and saw the comment when looked back at patches).. Thanks for the speedy fix @MaskRay !

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

if (ShlAmount > 0) {
     ShiftNode = CurDAG->getMachineNode(
         UBFMOpc, dl, VT, Op, CurDAG->getTargetConstant(ShrAmount, dl, VT),
         CurDAG->getTargetConstant(BitWidth - 1, dl, VT));
   }
 
   return SDValue(ShiftNode, 0);
 }
 
+// For bit-field-positioning pattern "(and (shl VAL, N), ShiftedMask)".
+static bool isBitfieldPositioningOpFromAnd(SelectionDAG *CurDAG, SDValue Op,
+                                           bool BiggerPattern,
+                                           const uint64_t NonZeroBits,
+                                           SDValue &Src, int &DstLSB,
+                                           int &Width);
+
+// For bit-field-positioning pattern "shl VAL, N)".
+static bool isBitfieldPositioningOpFromShl(SelectionDAG *CurDAG, SDValue Op,
+                                           bool BiggerPattern,
+                                           const uint64_t NonZeroBits,
+                                           SDValue &Src, int &DstLSB,
+                                           int &Width);
+
 /// Does this tree qualify as an attempt to move a bitfield into position,
-/// essentially "(and (shl VAL, N), Mask)".
+/// essentially "(and (shl VAL, N), Mask)" or (shl VAL, N).
 static bool isBitfieldPositioningOp(SelectionDAG *CurDAG, SDValue Op,
-                                    bool BiggerPattern,
-                                    SDValue &Src, int &ShiftAmount,
-                                    int &MaskWidth) {
+                                    bool BiggerPattern, SDValue &Src,
+                                    int &DstLSB, int &Width) {
   EVT VT = Op.getValueType();
   unsigned BitWidth = VT.getSizeInBits();
   (void)BitWidth;
   assert(BitWidth == 32 || BitWidth == 64);
 
   KnownBits Known = CurDAG->computeKnownBits(Op);
 
   // Non-zero in the sense that they're not provably zero, which is the key
   // point if we want to use this value
-  uint64_t NonZeroBits = (~Known.Zero).getZExtValue();
+  const uint64_t NonZeroBits = (~Known.Zero).getZExtValue();
+  if (!isShiftedMask_64(NonZeroBits))
+    return false;
 
-  // Discard a constant AND mask if present. It's safe because the node will
-  // already have been factored into the computeKnownBits calculation above.
-  uint64_t AndImm;
-  if (isOpcWithIntImmediate(Op.getNode(), ISD::AND, AndImm)) {
-    assert((~APInt(BitWidth, AndImm) & ~Known.Zero) == 0);
-    Op = Op.getOperand(0);
+  switch (Op.getOpcode()) {
+  default:
+    break;
+  case ISD::AND:
+    return isBitfieldPositioningOpFromAnd(CurDAG, Op, BiggerPattern,
+                                          NonZeroBits, Src, DstLSB, Width);
+  case ISD::SHL:
+    return isBitfieldPositioningOpFromShl(CurDAG, Op, BiggerPattern,
+                                          NonZeroBits, Src, DstLSB, Width);
   }
 
-  // Don't match if the SHL has more than one use, since then we'll end up
-  // generating SHL+UBFIZ instead of just keeping SHL+AND.
-  if (!BiggerPattern && !Op.hasOneUse())
+  return false;
+}
+
+static bool isBitfieldPositioningOpFromAnd(SelectionDAG *CurDAG, SDValue Op,
+                                           bool BiggerPattern,
+                                           const uint64_t NonZeroBits,
+                                           SDValue &Src, int &DstLSB,
+                                           int &Width) {
+  assert(isShiftedMask_64(NonZeroBits) && "Caller guaranteed");
+
+  EVT VT = Op.getValueType();
+  assert((VT == MVT::i32 || VT == MVT::i64) &&
+         "Caller guarantees VT is one of i32 or i64");
+
+  uint64_t AndImm;
+  if (!isOpcWithIntImmediate(Op.getNode(), ISD::AND, AndImm))
     return false;
 
+  // If (~AndImm & NonZeroBits) is not zero at POS, we know that
+  //   1) (AndImm & (1 << POS) == 0)
+  //   2) the result of AND is not zero at POS bit (according to NonZeroBits)
+  //
+  // 1) and 2) don't agree so something must be wrong (e.g., in
+  // 'SelectionDAG::computeKnownBits')
+  assert((~AndImm & NonZeroBits) == 0 &&
+         "Something must be wrong (e.g., in SelectionDAG::computeKnownBits)");
+
+  SDValue AndOp0 = Op.getOperand(0);
+
   uint64_t ShlImm;
-  if (!isOpcWithIntImmediate(Op.getNode(), ISD::SHL, ShlImm))
+  if (!isOpcWithIntImmediate(AndOp0.getNode(), ISD::SHL, ShlImm))
     return false;
-  Op = Op.getOperand(0);
 
-  if (!isShiftedMask_64(NonZeroBits))
+  // Bail out if the SHL has more than one use, since then we'll end up
+  // generating SHL+UBFIZ instead of just keeping SHL+AND.
+  if (!BiggerPattern && !AndOp0.hasOneUse())
     return false;
 
-  ShiftAmount = countTrailingZeros(NonZeroBits);
-  MaskWidth = countTrailingOnes(NonZeroBits >> ShiftAmount);
+  DstLSB = countTrailingZeros(NonZeroBits);
+  Width = countTrailingOnes(NonZeroBits >> DstLSB);
 
   // BFI encompasses sufficiently many nodes that it's worth inserting an extra
   // LSL/LSR if the mask in NonZeroBits doesn't quite match up with the ISD::SHL
   // amount.  BiggerPattern is true when this pattern is being matched for BFI,
   // BiggerPattern is false when this pattern is being matched for UBFIZ, in
   // which case it is not profitable to insert an extra shift.
-  if (ShlImm - ShiftAmount != 0 && !BiggerPattern)
+  if (ShlImm != DstLSB && !BiggerPattern)
+    return false;
+
+  Src = getLeftShift(CurDAG, AndOp0.getOperand(0), ShlImm - DstLSB);
+  return true;
+}
+
+static bool isBitfieldPositioningOpFromShl(SelectionDAG *CurDAG, SDValue Op,
+                                           bool BiggerPattern,
+                                           const uint64_t NonZeroBits,
+                                           SDValue &Src, int &DstLSB,
+                                           int &Width) {
+  assert(isShiftedMask_64(NonZeroBits) && "Caller guaranteed");
+
+  EVT VT = Op.getValueType();
+  assert((VT == MVT::i32 || VT == MVT::i64) &&
+         "Caller guarantees that type is i32 or i64");
+
+  uint64_t ShlImm;
+  if (!isOpcWithIntImmediate(Op.getNode(), ISD::SHL, ShlImm))
+    return false;
+
+  if (!BiggerPattern && !Op.hasOneUse())
+    return false;
+
+  DstLSB = countTrailingZeros(NonZeroBits);
+  Width = countTrailingOnes(NonZeroBits >> DstLSB);
+
+  if (DstLSB != ShlImm && !BiggerPattern)
     return false;
-  Src = getLeftShift(CurDAG, Op, ShlImm - ShiftAmount);
 
+  Src = getLeftShift(CurDAG, Op.getOperand(0), ShlImm - DstLSB);
   return true;
 }
 
 static bool isShiftedMask(uint64_t Mask, EVT VT) {
   assert(VT == MVT::i32 || VT == MVT::i64);
   if (VT == MVT::i32)
     return isShiftedMask_32(Mask);
   return isShiftedMask_64(Mask);
static bool tryBitfieldInsertOpFromOrAndImm(SDNode *N, SelectionDAG *CurDAG) {
   if (!isOpcWithIntImmediate(N, ISD::OR, OrImm))
     return false;
 
   // Skip this transformation if the ORR immediate can be encoded in the ORR.
   // Otherwise, we'll trade an AND+ORR for ORR+BFI/BFXIL, which is most likely
   // performance neutral.
   if (AArch64_AM::isLogicalImmediate(OrImm, BitWidth))
     return false;
 
   uint64_t MaskImm;
   SDValue And = N->getOperand(0);
   // Must be a single use AND with an immediate operand.
   if (!And.hasOneUse() ||
       !isOpcWithIntImmediate(And.getNode(), ISD::AND, MaskImm))
     return false;
 
   // Compute the Known Zero for the AND as this allows us to catch more general
   // cases than just looking for AND with imm.