[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I have minor comments, but otherwise I think this is nicely restricted as a start. Why isn't it added to the pass pipeline?
@fhahn @efriedma Any objections?



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:163
+    if (!isGuaranteedNotToBeUndefOrPoison(StepValue, PN, &DT)) {
+      unsigned OpIdx = SteppingInst->getOperand(1) == StepValue;
+      SteppingInst->setOperand(
----------------
aqjune wrote:
> fhahn wrote:
> > I think currently cases where we have multiple candidates for the same PHI are not handled correctly at the moment here, as after handling the first candidate the step operand will  not match the original StepValue. Multiple candidates would be added for a loop like the one below I think.
> > 
> > ```
> > loop:
> >   %i = phi i32 [%init, %entry], [%i.next, %loop]
> >   %i.fr = freeze i32 %i
> >   call void @call(i32 %i.fr)
> >   %i.next = add nsw nuw i32 %i, %step
> >   %i.next.fr = freeze i32 %i.next
> >   %cond = icmp eq i32 %i.next, %n
> >   call void @call(i32 %i.next.fr)
> >   br i1 %cond, label %loop, label %exit
> > ```
> Now this case will be correctly handled. Have a test add_multiuses2.
Can you also add:
```
loop:
  %i = phi i32 [%init, %entry], [%i.next, %loop]
  %i.fr1 = freeze i32 %i
  call void @call(i32 %i.fr1)
  %i.fr2 = freeze i32 %i
  call void @call(i32 %i.fr2)
  %i.next = add nsw nuw i32 %i, %step
  %cond = icmp eq i32 %i.next, %n
  br i1 %cond, label %loop, label %exit
```
and the double use of the frozen step value as well?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:45
+#include <map>
+#include <queue>
+
----------------
Not needed anymore?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:179
+    return false;
+  }
+
----------------
Nit: No braces.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:187
+
+    ProcessedPHIs.insert(Info.PHI);
+    BinaryOperator *StepI = Info.StepInst;
----------------
`if (!ProcessedPHIs.insert(Info.PHI))`
(or `insert(..).second`)


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:189
+    BinaryOperator *StepI = Info.StepInst;
+    assert(StepI);
+
----------------
Please add a message to asserts, also in other places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523