[PATCH] D79720: [IndVarSimplify][LoopUtils] Track rewrite cost per unique BB (PR45835)

[dmajor via Phabricator via llvm-commits]

dmajor created this revision.
dmajor added reviewers: lebedev.ri, reames, fhahn, efriedma.
dmajor added a project: LLVM.
Herald added a subscriber: hiraditya.
dmajor added a comment.
lebedev.ri added a reviewer: mkazantsev.
lebedev.ri retitled this revision from "[IndVarSimplify][LoopUtils] Track rewrite cost per unique BB" to "[IndVarSimplify][LoopUtils] Track rewrite cost per unique BB (PR45835)".
lebedev.ri edited the summary of this revision.

Note: the test case does not currently fail on trunk before this change (although it does fail on the 10.0 release branch, where I would eventually like to get this merged) because after D73744 <https://reviews.llvm.org/D73744> it is no longer blanket assumed that SCEVMinMaxExpr is high cost. Any pointers on how I could modify the test to get around that? (I'm not familiar with why this IR is considered to have a SCEVMinMaxExpr or what it would take to make it high cost.)


lebedev.ri added a comment.

Thank you for looking into it!

(In reply to dmajor from https://bugs.llvm.org/show_bug.cgi?id=45835#c3)

> For anyone curious where the different values of HighCost come from...
> 
> When we're recursively looking at the second NAry operand [0] of the first
>  value, that operand is considered high cost due to [1]:
> 
>   if (isa<SCEVMinMaxExpr>(S))
>     return true;
>    
> 
> When we're looking at the second NAry operand of the second value, it is
>  considered low cost due to [2]:
> 
>   // If we can find an existing value for this scev available at the point "At"
>   // then consider the expression cheap.
>   if (At && getRelatedExistingExpansion(S, At, L))
>     return false;

Yes, but it is still not obvious to me as to why that happens?
It's the same PHI node, we are asking about the same value, for the same basic block.
Why do we not find expansion first time but do find it second time?
Did we perform some expansion inbetween?

> [0]
>  https://github.com/llvm/llvm-project/blob/release/10.x/llvm/lib/Analysis/
>  ScalarEvolutionExpander.cpp#L2198-L2205
>  [1]
>  https://github.com/llvm/llvm-project/blob/release/10.x/llvm/lib/Analysis/
>  ScalarEvolutionExpander.cpp#L2193-L2196
>  [2]
>  https://github.com/llvm/llvm-project/blob/release/10.x/llvm/lib/Analysis/
>  ScalarEvolutionExpander.cpp#L2135-L2138




If a PHI node has multiple (identical) values for a given block, rewriteLoopExitValues must either rewrite all of them or none of them. Therefore, instead of tracking the cost for the individual values, track the cost per unique BB.

See PR45835 <https://bugs.llvm.org/show_bug.cgi?id=45835>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79720

Files:
  llvm/lib/Transforms/Utils/LoopUtils.cpp
  llvm/test/Transforms/IndVarSimplify/pr45835.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79720.263184.patch
Type: text/x-patch
Size: 4043 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200511/57d0621a/attachment-0001.bin>