[PATCH] D63885: [LOOPINFO] Introduce the loop guard API.

Wed Jul 17 09:55:34 PDT 2019

reames added a comment.

In D63885#1587492 <https://reviews.llvm.org/D63885#1587492>, @kbarton wrote:

> In D63885#1563245 <https://reviews.llvm.org/D63885#1563245>, @reames wrote:
>
> > I'm not sure an API this general is the most relevant.  I would have expected something more tightly tied to the loop itself instead of the region of code which contains the loop.  Thinking about how transforms might want to use this, I suspect a much tighter definition would be useful.
> >
> > Consider a transform which figures out a condition C under which loop L is a noop.  Modifying the guard (as defined here) would not be valid, but a guard which controls the preheader directly w/no side effects possible inside the guarded region, but outside the loop would be exactly what was needed.  Not sure what other transforms need off the top of my head, but I think that needs to be explored.
>
>
> @reames Could you expand a bit more on what you mean by "a guard which controls the preheader directly"? At first I took that to mean the guard should be the immediate dominator of the preheader, but now I'm not sure if that is what you meant.

I meant that we should focus on finding a guarding branch which directly controls entry into the loop preheader, not some arbitrary earlier branch which happens to avoid the whole section of the CFG.  I believe from the discussion, that the patch has evolved in this direction.

> I'm also not convinced that checking for side effects at the same time as trying to identify the guard is a good idea. To me they are very separate concepts, and combining them ends up confusing things.

I find the opposite very confusing, so we're even.

I'm about to do a run through the patch and review "fresh".  I have not been following all of the discussion, so it's possible I'll miss something which has already been discussed, but that way it's approachable without having to catch up on a bunch of back and forth.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885