[PATCH] D35010: [IRCE] Recognize loops with ne/eq latch conditions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:818
+      // while (true) {               while (true) {
+      //   if (++i == len)     --->     if (++i > len - 1)
+      //     break;                       break;
----------------
sanjoy wrote:
> I'm not sure why you need to handle this case differently than the above.  That is,
> 
> ```
> while (true) {
>   if (++i == len)
>     break;
> }
> ```
> 
> seems equivalent to
> 
> ```
> while (++i != len) {
> }
> ```
> 
The difference here is in `LatchBrExitIdx`. Here is a tricky moment. What we have here is
  loop:
    %cond = icmp eq i32 %iv.next, %len
    br i1 %cond, label %exit, label %loop
and
  loop:
    %cond = icmp ne i32 %iv.next, %len
    br i1 %cond, label %loop, label %exit
We could transform them both into
  loop:
    %cond = icmp le i32 %iv.next, %len
    br i1 %cond, label %loop, label %exit

And set `LatchBrExitIdx` to 1. But `LatchBrExitIdx` is then stored into a structure and used later. It means that what we should not just change value of `LatchBrExitIdx`, but also change actual successors of branch instruction. And we should either remember that we did this interchange and swap them in the end of the method, or swap them just after this replacement (risking to have CFG changes before we checked that the gates pass).

This approach looks more complex for understanding, and also looks more error-prone. That's why I separate these cases here.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:853
+      // it virtually when we replaced EQ with SGT.
+      if (!DecreasedRightValueByOne) {
+        IRBuilder<> B(Preheader->getTerminator());
----------------
sanjoy wrote:
> I'm missing a link here.  IIUC:
> 
>  - You started with the backedge taken condition as `!(++i == len)`
>  - `!(++i == len)` is equivalent to `!(++i >s (len - 1))`, after checking `len - 1` does not overflow (this is the new step you added)
>  - `!(++i >s (len - 1))` is equivalent to `(++i s<= (len - 1))`
>  - `(++i s<= (len - 1))` is equivalent to `(++i s< len)` after checking that `(len - 1) + 1` does not overflow
> 
> Where does `DecreasedRightValueByOne` come in?
In case 2. After we go from `(++i == length)` to `(++i >s (len - 1))`, the new `RighValue` we are working with is actually `len - 1`. We don't need to create such an instruction, though. `RightValue` is only used to calculate `RightValue + 1`. Knowing that we have decreased the old `RightValue` by 1, we could just not add this 1 and use the old value of `len` instead.


https://reviews.llvm.org/D35010