[PATCH] D71831: [PowerPC] Exploit the rldicl + rldicl when and with mask

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

In D71831#1938331 <https://reviews.llvm.org/D71831#1938331>, @steven.zhang wrote:

> In D71831#1936489 <https://reviews.llvm.org/D71831#1936489>, @nemanjai wrote:
>
> > 2. Rename `RotateRightClearLeft` to something like `RightJustifyRangeAndClear` as it appears that is what the function is doing.
>
>
> In ISA, the RLDICL is named as: "Rotate Left Doubleword Immediate then Clear Left". I am not sure if the right justify range make it more clear. Regarding to left or right, it is just a wrap rotate. The lambda here is trying to hide the detail that implement the right rotate with left rotate. Personally, I prefer the RotateRightClearLeft one, but also ok to the RightJustifyRangeAndClear if you insist.


I think we can retire this discussion quite easily by just removing the lambda. There isn't a whole lot of point to a lambda that is used only once. If it helped with readability I would be in favour of it, but I think it actually makes the code less readable.

In any case, I described the concise algorithm in pseudo-code not so much because I think it is a useful comment, but because I think that is how the function should flow. For example, isn't something like this much more readable:

  bool PPCDAGToDAGISel::tryAsPairOfRLDICL(SDNode *N) {
    assert(N->getOpcode() == ISD::AND && "ISD::AND SDNode expected");
    uint64_t Imm64;
  
    // Do nothing if it is 16-bit imm as the pattern in the .td file handles
    // it well with "andi.".
    if (!isInt64Immediate(N->getOperand(1).getNode(), Imm64) || isUInt<16>(Imm64))
      return false;
  
    SDLoc Loc(N);
    SDValue Val = N->getOperand(0);
  
    // Optimized with two rldicl's as follows:
    // Add missing bits on left to the mask and check that the mask is a
    // wrapped run of ones, i.e.
    // Change pattern |0001111100000011111111|
    //             to |1111111100000011111111|.
    unsigned NumOfLeadingZeros = countLeadingZeros(Imm64);
    if (NumOfLeadingZeros != 0)
      Imm64 |= maskLeadingOnes<uint64_t>(NumOfLeadingZeros);
    unsigned MB, ME;
    if (!isRunOfOnes64(Imm64, MB, ME))
      return false;
  
    //         ME     MB
    // +----------------------+     +----------------------+
    // |1111111100000011111111| ->  |0000001111111111111111|
    // +----------------------+     +----------------------+
    //  0                    63      0                    63
    // There are ME + 1 ones on the left and (MB - ME + 63) & 63 zeros in between.
    unsigned OnesOnLeft = ME + 1;
    unsigned ZerosInBetween = (MB - ME + 63) & 63;
  
    // Rotate left by OnesOnLeft (so leading ones are now trailing ones) and clear
    // on the left the bits that are already zeros in the mask.
    Val = SDValue(CurDAG->getMachineNode(PPC::RLDICL, Loc, MVT::i64, Val,
                                         getI64Imm(OnesOnLeft, Loc),
                                         getI64Imm(ZerosInBetween, Loc)),
                  0);
  
    //                                      ME     MB
    // +----------------------+     +----------------------+
    // |0000001111111111111111| ->  |0001111100000011111111|
    // +----------------------+     +----------------------+
    //  0                    63      0                    63
    // Rotate back by 64 - OnesOnLeft to undo previous rotate. Then clear on the
    // left the number of ones we previously added.
    SDValue Ops[] = {Val, getI64Imm(64 - OnesOnLeft, Loc),
                     getI64Imm(NumOfLeadingZeros, Loc)};
    CurDAG->SelectNodeTo(N, PPC::RLDICL, MVT::i64, Ops);
    return true;
  }

Note that the above is just a sample of how I think the code should be structured for readability. While I am fairly sure it is semantically equivalent to what you have, I did not carefully verify this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71831