[PATCH] D55604: [AggressiveInstCombine] convert rotate with guard branch into funnel shift (PR34924)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:148
+  // llvm.fshl.i32(i32 %X, i32 %RotAmt)
+  IRBuilder<> Builder(&Phi);
+  Function *F = Intrinsic::getDeclaration(Phi.getModule(), IID, Phi.getType());
----------------
nikic wrote:
> It looks like this is not the right way to replace a phi. For this test case:
> 
> ```
> define i32 @rotl(i32 %a, i32 %b) {
> entry:
>   %cmp = icmp eq i32 %b, 0
>   br i1 %cmp, label %end, label %rotbb
> 
> rotbb:
>   %sub = sub i32 32, %b
>   %shr = lshr i32 %a, %sub
>   %shl = shl i32 %a, %b
>   %or = or i32 %shr, %shl
>   br label %end
> 
> end:
>   %cond = phi i32 [ %or, %rotbb ], [ %a, %entry ]
>   %other = phi i32 [ 1, %rotbb ], [ 2, %entry ]
>   %res = or i32 %cond, %other
>   ret i32 %res
> }
> ```
> 
> I get:
> 
> ```
> PHI nodes not grouped at top of basic block!
>   %other = phi i32 [ 1, %rotbb ], [ 2, %entry ]
> label %end
> in function rotl
> LLVM ERROR: Broken function found, compilation aborted!
> ```
> 
> Some possible alternatives from `git grep "IRBuilder.*Phi"`:
> 
> ```
> lib/Transforms/Instrumentation/GCOVProfiling.cpp:          IRBuilder<> BuilderForPhi(&*BB.begin());
> lib/Transforms/Scalar/IndVarSimplify.cpp:        IRBuilder<> Builder(&*WidePhi->getParent()->getFirstInsertionPt());
> lib/Transforms/Utils/BypassSlowDivision.cpp:  IRBuilder<> Builder(PhiBB, PhiBB->begin());
> ```
Yes, good catch. I'll add a regression test and fix that.


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

https://reviews.llvm.org/D55604