[NFC][PowerPC] Add a function tryAndWithMask

[steven.zhang]

Summary

I plan to add some more logic to optimize the code gen for "and" with mask. So, need to move all these logic into a function first and extend it later.

[shchenz]

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4685–4686

Is there any reason lines 4678 ~ 4686 are left out from tryAndWithMask()?

[steven.zhang]

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4685–4686

It is rotate and mask, not and mask.

[shchenz]

LGTM. Some minor comment, you can handle it as you like.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

207

Maybe tryAndWithImmMask?

4685–4686

I guess here we want to handle special case for And with a Imm mask, so if we change name tryAndWithMask() to tryAndWithImmMask, maybe we can also put these lines to that function also. What do you think? I am also ok to leave it as now.

[steven.zhang]

I realize that most reviewers were on vacation during this patch was reviewed and committed. It is a bit too haste to commit this patch. Please add the comments here if have any concern. Sorry about this.

[nemanjai]

I think this refactoring doesn't really do anything to simplify the code or make it more readable. It just takes fairly complex logic and moves it into a separate function. Then subsequent patches will add even further complexity to this function. Overall, there isn't much of a gain from the refactoring.
What I think would make sense is to simplify the tryAndWithMask() to something conceptually similar to the following:

tryAndWithMask(...) {
  if (canBeCodeGenedAsSingleRLWINM())
    return produceRLWINMSDNode();
  if (canBeCodeGenedAsSingleRLWIMI())
    return produceRLWIMISDNode();
  if (canBeCodeGenedAsSingleRLDICL())
    return produceRLDICLSDNode();
  if (canBeCodeGenedAsPairOfRLDICL())
    return produceRLDICLPairSDNode();
  ...
  return SDValue();

Then the top level function would call this function to create an SDValue and return it if it is non-empty or break out of the switch if it is empty.

[NeHuang]

Though the logic in this new function looks consistent with original code with optimized condition check, IMHO it is still not that readable to me. Agree with @nemanjai 's suggestion, adding conceptual functions for"SingleRLWINM", "SingleRLWIMI", "SingleRLDICL" and "PairOfRLDICL" will make it more readable. Also added comments for variable re-def issues.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4361

N->getOperand(0) is used at line 4361, 4362, 4373, 4388, 4389, 4404, 4405), and it is defined in line 4354. Can we use the defined variable (better name if necessary) to replace "N->getOperand(0)" ?

4362

Re-definition of Val. (Originally defined in Line 4354)

4402

Re-definition of MB and ME. (Originally defined in line 4355)

[steven.zhang]

Yes, it is just moving the code into a new function to avoid too much code in the switch case. And it is definitely more clear if moving the condition into small functions. As there are several different conditions that could produce the same instruction with different arguments. I will do it as follows.

bool tryAndWithMask(...) {
   
   if (tryCodeGenAsSingleRLWINM())
       return true;
   if (tryCodeGenAsPairRLWINM())
      return true;
   ...
}

Maybe, the tryAndWithMask is NOT needed if we move the small pieces of code into functions.

[amyk]

One minor comment from me. I also agree with what has been stated previously (to try to simplify tryAndWithMask()).

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4349

If we are to add a new function, I think it is beneficial to add some doxygen comments for the function.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

public:
 
     // Select - Convert the specified operand from a target-independent to a
     // target-specific node if it hasn't already been changed.
     void Select(SDNode *N) override;
 
     bool tryBitfieldInsert(SDNode *N);
     bool tryBitPermutation(SDNode *N);
     bool tryIntCompareInGPR(SDNode *N);
+    bool tryAndWithMask(SDNode *N);
 
     // tryTLSXFormLoad - Convert an ISD::LOAD fed by a PPCISD::ADD_TLS into
     // an X-Form load instruction with the offset being a relocation coming from
     // the PPCISD::ADD_TLS.
     bool tryTLSXFormLoad(LoadSDNode *N);
     // tryTLSXFormStore - Convert an ISD::STORE fed by a PPCISD::ADD_TLS into
     // an X-Form store instruction with the offset being a relocation coming from
     // the PPCISD::ADD_TLS.
static bool mayUseP9Setb(SDNode *N, const ISD::CondCode &CC, SelectionDAG *DAG,
   }
 
   LLVM_DEBUG(dbgs() << "Found a node that can be lowered to a SETB: ");
   LLVM_DEBUG(N->dump());
 
   return true;
 }
 
+bool PPCDAGToDAGISel::tryAndWithMask(SDNode *N) {
+  if (N->getOpcode() != ISD::AND)
+    return false;
+
+  SDLoc dl(N);
+  SDValue Val = N->getOperand(0);
+  unsigned Imm, Imm2, SH, MB, ME;
+  uint64_t Imm64;
+
+  // If this is an and of a value rotated between 0 and 31 bits and then and'd
+  // with a mask, emit rlwinm
+  if (isInt32Immediate(N->getOperand(1), Imm) &&
+      isRotateAndMask(N->getOperand(0).getNode(), Imm, false, SH, MB, ME)) {
+    SDValue Val = N->getOperand(0).getOperand(0);
+    SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl),
+                      getI32Imm(ME, dl) };
+    CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops);
+    return true;
+  }
+
+  // If this is just a masked value where the input is not handled, and
+  // is not a rotate-left (handled by a pattern in the .td file), emit rlwinm
+  if (isInt32Immediate(N->getOperand(1), Imm)) {
+    if (isRunOfOnes(Imm, MB, ME) &&
+        N->getOperand(0).getOpcode() != ISD::ROTL) {
+      SDValue Ops[] = { Val, getI32Imm(0, dl), getI32Imm(MB, dl),
+                        getI32Imm(ME, dl) };
+      CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops);
+      return true;
+    }
+    // AND X, 0 -> 0, not "rlwinm 32".
+    if (Imm == 0) {
+      ReplaceUses(SDValue(N, 0), N->getOperand(1));
+      return true;
+    }
+
+    // ISD::OR doesn't get all the bitfield insertion fun.
+    // (and (or x, c1), c2) where isRunOfOnes(~(c1^c2)) might be a
+    // bitfield insert.
+    if (N->getOperand(0).getOpcode() == ISD::OR &&
+        isInt32Immediate(N->getOperand(0).getOperand(1), Imm2)) {
+      // The idea here is to check whether this is equivalent to:
+      //   (c1 & m) | (x & ~m)
+      // where m is a run-of-ones mask. The logic here is that, for each bit in
+      // c1 and c2:
+      //  - if both are 1, then the output will be 1.
+      //  - if both are 0, then the output will be 0.
+      //  - if the bit in c1 is 0, and the bit in c2 is 1, then the output will
+      //    come from x.
+      //  - if the bit in c1 is 1, and the bit in c2 is 0, then the output will
+      //    be 0.
+      //  If that last condition is never the case, then we can form m from the
+      //  bits that are the same between c1 and c2.
+      unsigned MB, ME;
+      if (isRunOfOnes(~(Imm^Imm2), MB, ME) && !(~Imm & Imm2)) {
+        SDValue Ops[] = { N->getOperand(0).getOperand(0),
+                            N->getOperand(0).getOperand(1),
+                            getI32Imm(0, dl), getI32Imm(MB, dl),
+                            getI32Imm(ME, dl) };
+        ReplaceNode(N, CurDAG->getMachineNode(PPC::RLWIMI, dl, MVT::i32, Ops));
+        return true;
+      }
+    }
+  } else if (isInt64Immediate(N->getOperand(1).getNode(), Imm64)) {
+    // If this is a 64-bit zero-extension mask, emit rldicl.
+    if (isMask_64(Imm64)) {
+      MB = 64 - countTrailingOnes(Imm64);
+      SH = 0;
+
+      if (Val.getOpcode() == ISD::ANY_EXTEND) {
+        auto Op0 = Val.getOperand(0);
+        if ( Op0.getOpcode() == ISD::SRL &&
+           isInt32Immediate(Op0.getOperand(1).getNode(), Imm) && Imm <= MB) {
+
+           auto ResultType = Val.getNode()->getValueType(0);
+           auto ImDef = CurDAG->getMachineNode(PPC::IMPLICIT_DEF, dl,
+                                               ResultType);
+           SDValue IDVal (ImDef, 0);
+
+           Val = SDValue(CurDAG->getMachineNode(PPC::INSERT_SUBREG, dl,
+                         ResultType, IDVal, Op0.getOperand(0),
+                         getI32Imm(1, dl)), 0);
+           SH = 64 - Imm;
+        }
+      }
+
+      // If the operand is a logical right shift, we can fold it into this
+      // instruction: rldicl(rldicl(x, 64-n, n), 0, mb) -> rldicl(x, 64-n, mb)
+      // for n <= mb. The right shift is really a left rotate followed by a
+      // mask, and this mask is a more-restrictive sub-mask of the mask implied
+      // by the shift.
+      if (Val.getOpcode() == ISD::SRL &&
+          isInt32Immediate(Val.getOperand(1).getNode(), Imm) && Imm <= MB) {
+        assert(Imm < 64 && "Illegal shift amount");
+        Val = Val.getOperand(0);
+        SH = 64 - Imm;
+      }
+
+      SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl) };
+      CurDAG->SelectNodeTo(N, PPC::RLDICL, MVT::i64, Ops);
+      return true;
+    } else if (isMask_64(~Imm64)) {
+      // If this is a negated 64-bit zero-extension mask,
+      // i.e. the immediate is a sequence of ones from most significant side
+      // and all zero for reminder, we should use rldicr.
+      MB = 63 - countTrailingOnes(~Imm64);
+      SH = 0;
+      SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl) };
+      CurDAG->SelectNodeTo(N, PPC::RLDICR, MVT::i64, Ops);
+      return true;
+    }
+  }
+
+  return false;
+}
+
 // Select - Convert the specified operand from a target-independent to a
 // target-specific node if it hasn't already been changed.
 void PPCDAGToDAGISel::Select(SDNode *N) {
   SDLoc dl(N);
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);
     return;   // Already selected.
   }
if (Offset.getOpcode() == ISD::TargetConstant ||
       SDNode *MN = CurDAG->getMachineNode(
           Opcode, dl, LD->getValueType(0),
           PPCLowering->getPointerTy(CurDAG->getDataLayout()), MVT::Other, Ops);
       transferMemOperands(N, MN);
       ReplaceNode(N, MN);
       return;
     }
   }
 
-  case ISD::AND: {
-    unsigned Imm, Imm2, SH, MB, ME;
-    uint64_t Imm64;
-
-    // If this is an and of a value rotated between 0 and 31 bits and then and'd
-    // with a mask, emit rlwinm
-    if (isInt32Immediate(N->getOperand(1), Imm) &&
-        isRotateAndMask(N->getOperand(0).getNode(), Imm, false, SH, MB, ME)) {
-      SDValue Val = N->getOperand(0).getOperand(0);
-      SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl),
-                        getI32Imm(ME, dl) };
-      CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops);
-      return;
-    }
-    // If this is just a masked value where the input is not handled above, and
-    // is not a rotate-left (handled by a pattern in the .td file), emit rlwinm
-    if (isInt32Immediate(N->getOperand(1), Imm) &&
-        isRunOfOnes(Imm, MB, ME) &&
-        N->getOperand(0).getOpcode() != ISD::ROTL) {
-      SDValue Val = N->getOperand(0);
-      SDValue Ops[] = { Val, getI32Imm(0, dl), getI32Imm(MB, dl),
-                        getI32Imm(ME, dl) };
-      CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops);
-      return;
-    }
-    // If this is a 64-bit zero-extension mask, emit rldicl.
-    if (isInt64Immediate(N->getOperand(1).getNode(), Imm64) &&
-        isMask_64(Imm64)) {
-      SDValue Val = N->getOperand(0);
-      MB = 64 - countTrailingOnes(Imm64);
-      SH = 0;
-
-      if (Val.getOpcode() == ISD::ANY_EXTEND) {
-        auto Op0 = Val.getOperand(0);
-        if ( Op0.getOpcode() == ISD::SRL &&
-           isInt32Immediate(Op0.getOperand(1).getNode(), Imm) && Imm <= MB) {
-
-           auto ResultType = Val.getNode()->getValueType(0);
-           auto ImDef = CurDAG->getMachineNode(PPC::IMPLICIT_DEF, dl,
-                                               ResultType);
-           SDValue IDVal (ImDef, 0);
-
-           Val = SDValue(CurDAG->getMachineNode(PPC::INSERT_SUBREG, dl,
-                         ResultType, IDVal, Op0.getOperand(0),
-                         getI32Imm(1, dl)), 0);
-           SH = 64 - Imm;
-        }
-      }
-
-      // If the operand is a logical right shift, we can fold it into this
-      // instruction: rldicl(rldicl(x, 64-n, n), 0, mb) -> rldicl(x, 64-n, mb)
-      // for n <= mb. The right shift is really a left rotate followed by a
-      // mask, and this mask is a more-restrictive sub-mask of the mask implied
-      // by the shift.
-      if (Val.getOpcode() == ISD::SRL &&
-          isInt32Immediate(Val.getOperand(1).getNode(), Imm) && Imm <= MB) {
-        assert(Imm < 64 && "Illegal shift amount");
-        Val = Val.getOperand(0);
-        SH = 64 - Imm;
-      }
-
-      SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl) };
-      CurDAG->SelectNodeTo(N, PPC::RLDICL, MVT::i64, Ops);
-      return;
-    }
-    // If this is a negated 64-bit zero-extension mask,
-    // i.e. the immediate is a sequence of ones from most significant side
-    // and all zero for reminder, we should use rldicr.
-    if (isInt64Immediate(N->getOperand(1).getNode(), Imm64) &&
-        isMask_64(~Imm64)) {
-      SDValue Val = N->getOperand(0);
-      MB = 63 - countTrailingOnes(~Imm64);
-      SH = 0;
-      SDValue Ops[] = { Val, getI32Imm(SH, dl), getI32Imm(MB, dl) };
-      CurDAG->SelectNodeTo(N, PPC::RLDICR, MVT::i64, Ops);
-      return;
-    }
-
-    // AND X, 0 -> 0, not "rlwinm 32".
-    if (isInt32Immediate(N->getOperand(1), Imm) && (Imm == 0)) {
-      ReplaceUses(SDValue(N, 0), N->getOperand(1));
-      return;
-    }
-    // ISD::OR doesn't get all the bitfield insertion fun.
-    // (and (or x, c1), c2) where isRunOfOnes(~(c1^c2)) might be a
-    // bitfield insert.
-    if (isInt32Immediate(N->getOperand(1), Imm) &&
-        N->getOperand(0).getOpcode() == ISD::OR &&
-        isInt32Immediate(N->getOperand(0).getOperand(1), Imm2)) {
-      // The idea here is to check whether this is equivalent to:
-      //   (c1 & m) | (x & ~m)
-      // where m is a run-of-ones mask. The logic here is that, for each bit in
-      // c1 and c2:
-      //  - if both are 1, then the output will be 1.
-      //  - if both are 0, then the output will be 0.
-      //  - if the bit in c1 is 0, and the bit in c2 is 1, then the output will
-      //    come from x.
-      //  - if the bit in c1 is 1, and the bit in c2 is 0, then the output will
-      //    be 0.
-      //  If that last condition is never the case, then we can form m from the
-      //  bits that are the same between c1 and c2.
-      unsigned MB, ME;
-      if (isRunOfOnes(~(Imm^Imm2), MB, ME) && !(~Imm & Imm2)) {
-        SDValue Ops[] = { N->getOperand(0).getOperand(0),
-                            N->getOperand(0).getOperand(1),
-                            getI32Imm(0, dl), getI32Imm(MB, dl),
-                            getI32Imm(ME, dl) };
-        ReplaceNode(N, CurDAG->getMachineNode(PPC::RLWIMI, dl, MVT::i32, Ops));
-        return;
-      }
-    }
+  case ISD::AND:
+    // If this is an 'and' with a mask, try to emit rlwinm/rldicl/rldicr
+    if (tryAndWithMask(N))
 
     // Other cases are autogenerated.
     break;
-  }
   case ISD::OR: {
     if (N->getValueType(0) == MVT::i32)
       if (tryBitfieldInsert(N))
         return;
 
     int16_t Imm;
     if (N->getOperand(0)->getOpcode() == ISD::FrameIndex &&
         isIntS16Immediate(N->getOperand(1), Imm)) {