[PATCH] D86005: [NewPM][LoopFullUnroll] Make LoopFullUnrollPass required

Mon Aug 17 17:48:02 PDT 2020

ychen added a comment.

In D86005#2222884 <https://reviews.llvm.org/D86005#2222884>, @aeubanks wrote:

> In D86005#2221998 <https://reviews.llvm.org/D86005#2221998>, @ychen wrote:
>
>> This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.
>
> Do you mean don't have the optnone pass instrumentation say that the pass should be skipped if it sees the proper metadata?

Yes, that's what I meant because we're overriding optnone's decision based on "llvm.loop.unroll.full" here. The patch in its current shape means the three passes are immune to the `optnone` which seems stronger than necessary.

> I think the metadata is specific just to this one pass and checking it doesn't belong in the optnone pass instrumentation.

Yeah, it is a little bit out of place. I'm running out of idea here. However, putting the logic there is very explicit about the intention.

> There may be some semantic meaning to fully unrolling a loop, and that should happen regardless of optnone/opt-bisect, whatever else.

Fully unrolling loops is just a regular transformation pass. It happens to have a user knob that should be respected (pragma). I could not think of other scenarios that it is special but I'm not sure. @asbirlea thoughts?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86005