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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think this would be immensely more readable if you use more descriptive names for variables. It is very hard to get this bit manipulation right in ones head so I really think you should try your best to make this as simple to follow as possible:

1. You use one mask in on line 4463 and then a different one on 4476. Please don't do this. Stick with a consistent example.
2. Rename `RotateRightClearLeft` to something like `RightJustifyRangeAndClear` as it appears that is what the function is doing.
3. Get rid of all the expressions involving ME/MB - especially things like `<imm> +/- MB/ME` as they are very difficult to reason about. For readability, favour defining temporary values just so they would have a name. For example: `MB+63-ME` is kind of meaningless to a reader. But if you do something like `unsigned FirstBitSetWhenRightJustified = MB + 63 - ME;` that is now much easier to follow. I realize that we are creating single-use temporaries this way, but I really think it is worth it for readability.

The algorithm appears to be something along the lines of:

  if (!MaskIsTwoContiguousRunsOfOnes)
    return
  // Add Missing Bits On Left To The Mask
  // Right Justify Mask And Clear Bits Formerly In The Middle
  // Rotate Back And Clear Bits Previously Added On Left

And I think the comments, function names and variable names should make it clear that this is what is happening.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4456
+  // Right rotate M bits, and clear left N bits
+  auto RotateRightClearLeft = [&](SDValue Val, int M, int N) {
+    return SDValue(CurDAG->getMachineNode(PPC::RLDICL, Loc, MVT::i64, Val,
----------------
I think it is confusing for a function called `RotateRightClearLeft` to emit a "Rotate Left Clear Left"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71831