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

Mon Mar 23 20:09:27 PDT 2020

steven.zhang added a comment.

In D71831#1936489 <https://reviews.llvm.org/D71831#1936489>, @nemanjai wrote:

> 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.

Good catch.

> 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.

> 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.

It is always a balance :) Agree that use the temp variable here as the rotate logic is indeed hard to follow.

> 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.

That is nice. Thank you.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71831