LiveIntervals verifier errors not caught when using -run-pass

[arsenm]

Bugzilla Link	46873
Version	trunk
OS	Linux
CC	@qcolombet,@yuanfang-chen

Extended Description

When using -run-pass with a single pass, LiveIntervals is not available in the final verifier run, and thus does not catch broken live intervals introduced by the pass. This seems to only work correctly only if another non-verifier pass is forced to run after the pass you are trying to observe.

A workaround is to either force a run of another pass that needs LiveIntervals after the interesting pass, or to use -start-before/-start-after around the pass

e.g.
llc -run-pass=breaks-liveints -verify-machineinstrs foo.mir // No verifier error
llc -run-pass=breaks-liveints,somethingelse -verify-machineinstrs // Verifier error
llc -start-before=breaks-liveints -stop-after=somethingelse -verify-machineinstrs // Verifier error

[arsenm]

I'm guessing this is because the pass manager is aggressively discarding the analyses after it's not required

[qcolombet]

I'm guessing this is because the pass manager is aggressively discarding the
analyses after it's not required

Correct!

[arsenm]

I'm guessing this is because the pass manager is aggressively discarding the
analyses after it's not required

Correct!

This is still a bug. The verifier still should use any analyses that are preserved by the pass, and the ordinary discard logic should not apply. The intent of -run-pass is to catch bugs in individual MIR passes, and this harms that functionality.

[qcolombet]

I'm guessing this is because the pass manager is aggressively discarding the
analyses after it's not required

Correct!

This is still a bug. The verifier still should use any analyses that are
preserved by the pass, and the ordinary discard logic should not apply. The
intent of -run-pass is to catch bugs in individual MIR passes, and this
harms that functionality.

Unfortunately, this is an issue with how the pass manager works. The verifier is just a regular pass and does not have any special logic to keep things alive.

If we want to fix this we should expose a new feature in the pass manager. Right now the verifier uses getAnalysisIfAvailable, which is not enough to keep the live-intervals alive is they are not used later on.

I guess we could try to temper with the pass pipeline to add a fake use of the live-intervals when the verifier is in use, but that's a slippery slope: how do you choose which analysis pass to keep alive and when?
Indeed, adding unconditionally things is not correct either: at one point the live-intervals pass may be actually dead and may not be recomputable.

Bottom line, to me this is not a bug, this is just how the pass manager works.

One mitigation we could use is adding an option like verify-live-intervals that would have the verifier issue an error if the live-intervals analysis is not available (or just make it a required pass within the verifier.)

What do you think?

[arsenm]

One mitigation we could use is adding an option like verify-live-intervals
that would have the verifier issue an error if the live-intervals analysis
is not available (or just make it a required pass within the verifier.)

What do you think?

This would be an improvement over having to run some other pass, but I worry adding another one of these random -verify flags isn't very discoverable. Is this something the new pass manager would avoid? If it treats analyses as something for a later pass to invalidate, would it still be eagerly discarded?

[qcolombet]

Is this something the new pass manager would avoid?

No idea but I doubt it.

If it treats analyses as
something for a later pass to invalidate, would it still be eagerly
discarded?

I would believe so, but I haven’t looked into the npm internals so not sure.

[yuanfang-chen]

One mitigation we could use is adding an option like verify-live-intervals
that would have the verifier issue an error if the live-intervals analysis
is not available (or just make it a required pass within the verifier.)

What do you think?

This would be an improvement over having to run some other pass, but I worry
adding another one of these random -verify flags isn't very discoverable. Is
this something the new pass manager would avoid?

I think NPM does not have this issue. AFAIK NPM has no discard mechansim. If you say getAnalysis, it is computed and saved. Next possible invalidation happens when next pass say it is not preserved. The opt commandline looks like these in llvm/test/Analysis/BasicAA/invalidation.ll

[yuanfang-chen]

FTR, D84959 mentioned no discarding may cause performance issue in NPM. That is opt pipeline. We will need to evaluate this after codegen is using NPM.

[jayfoad]

https://reviews.llvm.org/D129208 should fix this.