[PATCH] D86696: [Attributor][WIP] Introduce Loop AA

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.
Herald added a subscriber: danielkiss.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7943
+        if (auto *BI = dyn_cast<BranchInst>(&I)) {
+          auto *Op = BI->getOperand(0);
+          const auto &OpAA =
----------------
uenoku wrote:
> baziotis wrote:
> > bbn wrote:
> > > uenoku wrote:
> > > > baziotis wrote:
> > > > > uenoku wrote:
> > > > > > bbn wrote:
> > > > > > > baziotis wrote:
> > > > > > > > uenoku wrote:
> > > > > > > > > baziotis wrote:
> > > > > > > > > > For that to make sense, you have to verify that the branch `isConditional()`. Also, note that loops
> > > > > > > > > > may be [[ https://llvm.org/docs/LoopTerminology.html#rotated-loops | rotated ]]. In that case, the condition is not in the header block, but in the latch.
> > > > > > > > > > 
> > > > > > > > > > Generally, loops can get quite complicated and so, as Kuter said, you probably want to use
> > > > > > > > > > SCEV (for example, backedge taken count etc.) I'll have to think that again because SCEV
> > > > > > > > > > won't benefit from Attributor, meaning, at a high-level, using constant range AA when it makes
> > > > > > > > > > sense seems reasonable.
> > > > > > > > > FWIW, AAConstantRange actually uses SCEV internally but I agree that using AAConstantRange might make things complicated as a starting point. So you can use SCEV first. 
> > > > > > > > Yes, but the point is that using constant range directly makes things difficult because we have to re-deduce information and loop structure that SCEV is supposed to handle (e.g. the code above is doing something similar to backedge taken count).
> > > > > > > Thanks for the idea.
> > > > > > > 1. I think even if the loop is rotated, we can use such method to determine whether a loop
> > > > > > >     is endless or not, right?
> > > > > > > 2. I think SCEV could be a good idea, but I cannot find a reasonable test for this. The range I
> > > > > > >     get is always "full-set", can you give an example for this? Thanks
> > > > > > > I think SCEV could be a good idea, but I cannot find a reasonable test for this. The range I get is always "full-set", can you give an example for this? 
> > > > > > We have tests for loop terminations in a test for `willreturn` (I guess)
> > > > > 1) Well, it gets really complicated (and hacky) really fast. You have to have two code paths; one for rotated and one for not. Also, I agree with Kuter in the other comment. For an "always true" condition, the range has to be _equal_ to 1, not just contain it.
> > > > > 
> > > > > But even with an always true condition, you're not sure the loop is endless.
> > > > > Because the loop might have some kind of control
> > > > > flow inside that makes the loop stop. For example:
> > > > > ```
> > > > > while (some condition that is always true) {
> > > > >   if (some condition that makes the loop stop at some point)
> > > > >     break;
> > > > > }
> > > > > ```
> > > > > Which brings us again to the initial point: It seems using SCEV is the only sane way. SCEV is
> > > > > supposed to handle all that and other analyses / transformations to use it, instead of replicating
> > > > > part of its functionality.
> > > > > 
> > > > > 2) 
> > > > > > I think SCEV could be a good idea, but I cannot find a reasonable test for this
> > > > > 
> > > > > For what ?
> > > > > 
> > > > > > The range I get is always "full-set", can you give an example for this?
> > > > > 
> > > > > @uenoku should be able to help you here.
> > > > > I think even if the loop is rotated, we can use such method to determine whether a loop is endless or not, right
> > > > Maybe we can first check the loop is canonicalized or not. If the loop is LCSSA, things get easier. 
> > > > But as @baziotis says we need to replicate the code so it's not clear we should do similar things here.
> > > > One solution would be adding callbacks to SCEV and using Attributor information from SCEV but it would be (really) complicated and difficult. 
> > > Thanks for the idea here. I will take a look at it.
> > > Maybe we can first check the loop is canonicalized or not. If the loop is LCSSA, things get easier.
> > 
> > How would LCSSA help ? Btw, @bbn if you want to read about LCSSA or other canonical forms, you can check this: https://llvm.org/docs/LoopTerminology.html
> > 
> > > One solution would be adding callbacks to SCEV and using Attributor information from SCEV but it would be (really) complicated and difficult.
> > 
> > I mean yes, ideally SCEV would do its thing by taking advantage of the Attributor. But right now, it's certainly a bad idea unless we have a good plan for it.
> > 
> I thought we can use SCEV more easily and discuss the exit conditions more simply by restricting the loop form. However, Come to think of it,  LCSSA itself doesn't help the dedication. Sorry for confusion;)
> 
> > I mean yes, ideally SCEV would do its thing by taking advantage of the Attributor. But right now, it's certainly a bad idea unless we have a good plan for it.
> Yes, I agree
Great and np. So, @bbn you can try SCEV and don't hesitate to ask if you have any problem. SCEV is a beast, I know from experience :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86696