[PATCH] D71831: [PowerPC] Exploit the rldicl + rldicl when and with mask
Nemanja Ivanovic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 23 04:21:03 PDT 2020
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
More information about the llvm-commits
mailing list