This is an archive of the discontinued LLVM Phabricator instance.

Track validity of pass results
ClosedPublic

Authored by sepavloff on Nov 28 2016, 9:50 PM.

Details

Summary

This change introduces a flag in the Pass class to mark passes that have
valid results. The flag is set when the pass is run and cleared if it is
skipped or freed.

The flag allows to avoid verification when it is not possible. With the change more
than 500 failing test now pass in the builds with expensive checks enabled.

Event Timeline

sepavloff updated this revision to Diff 79509.Nov 28 2016, 9:50 PM
sepavloff retitled this revision from to Do not verify skipped pass.
sepavloff updated this object.
sepavloff added a reviewer: davide.
sepavloff added a subscriber: llvm-commits.
sepavloff updated this revision to Diff 80227.Dec 4 2016, 9:10 PM

Made use of the availability flag more consistent.

sepavloff updated this revision to Diff 80253.Dec 5 2016, 5:10 AM

Adapted a test to check loop verification.

sepavloff retitled this revision from Do not verify skipped pass to Track validity of pass results.Dec 5 2016, 5:12 AM
sepavloff updated this object.
davide edited edge metadata.Dec 5 2016, 6:35 AM
davide added a subscriber: chandlerc.

As this touches the internals of the pass manager, I'll be more confident to see this one going in if @chandlerc reviews it.

chandlerc edited edge metadata.Dec 5 2016, 9:07 AM

Generally I like where this is going, especially as it gives us a nice assert for getAnalysis that would have cought several bugs for me.

However, I'd really like to isolate this for analysis passes only so that we don't have to do so much with skipFunction. Is there no way to do that? Relatedly, the logic for when to set things available doesn't really make sense in the current patch. Might just need comments, but it seems very non-obvious when things become available.

include/llvm/Pass.h
86

I wouldn't put the doxygen comment here (or use end-of-line doxygen generally). Instead see my comment below...

97

This needs a doxygen comment as much or more than the rest. =] I also wouldn't try to group with the above method.

99–102

Does this need to be virtual? It seems a waste just for immutable passes. Is it even necessary there? It seems like those should be handled by the existing logic...

Also, I would find a 'setAvailable' without a default argument more clear. Or two functions, one to mark and one to clear.

lib/Analysis/LoopInfo.cpp
730–734

This looks like it is working around a bug. We should always have domtree when we are running LoopInfo...

lib/Analysis/LoopPass.cpp
178

I don't understand why passes are being marked available here?

sepavloff updated this revision to Diff 80540.Dec 6 2016, 9:41 PM
sepavloff edited edge metadata.

Addressed reviewer's notes.

sepavloff marked 3 inline comments as done.Dec 6 2016, 10:11 PM

However, I'd really like to isolate this for analysis passes only so that we don't have to do so much with skipFunction.

This facility indeed is used only for analysis passes. However analysis passes share the same code path with non-analysis ones and it is easier to maintain the flag for all passes. Besides there is no simple way to know if this particular pass is an analysis. There is a comment to PassInfo that states it can be obtained from a Pass by getPassInfo (https://github.com/llvm-mirror/llvm/blob/master/include/llvm/PassInfo.h#L28-L29), however such method does not exist in Pass.

On the other hand the new flag is not tied to analysis only, actually it tracks the fact whether the pass was executed or not. I renamed the flag to Executed and associated function accordingly in hope that the code becomes clearer.

Relatedly, the logic for when to set things available doesn't really make sense in the current patch. Might just need comments, but it seems very non-obvious when things become available.

When the flag must change is described in the comment to the method setExecuted.

include/llvm/Pass.h
99–102

The method is no more virtual. It however required a check in freePass to avoid invalidation of immutable passes. There is no default argument now.

lib/Analysis/LoopInfo.cpp
730–734

This is not about running LoopInfo, it is about *verification* of it. This method are run by a pass that clams it preserves LoopInfo. The LoopInfo results are valid at this point, but DomTree may be already freed because the last user of it has already executed.

Generally we have at least three options here:

  • skip verification if DomTree is not available,
  • do not free unneeded passes if verification is required,
  • make passes transitively dependent on passes required for verification.

Of these options the first is the most obvious and the least pervasive.

lib/Analysis/LoopPass.cpp
178

Unneeded change. Removed.

However, I'd really like to isolate this for analysis passes only so that we don't have to do so much with skipFunction.

This facility indeed is used only for analysis passes. However analysis passes share the same code path with non-analysis ones and it is easier to maintain the flag for all passes. Besides there is no simple way to know if this particular pass is an analysis. There is a comment to PassInfo that states it can be obtained from a Pass by getPassInfo (https://github.com/llvm-mirror/llvm/blob/master/include/llvm/PassInfo.h#L28-L29), however such method does not exist in Pass.

On the other hand the new flag is not tied to analysis only, actually it tracks the fact whether the pass was executed or not. I renamed the flag to Executed and associated function accordingly in hope that the code becomes clearer.

Relatedly, the logic for when to set things available doesn't really make sense in the current patch. Might just need comments, but it seems very non-obvious when things become available.

When the flag must change is described in the comment to the method setExecuted.

ping @chandlerc

davide accepted this revision.Dec 22 2016, 9:29 AM
davide edited edge metadata.

I looked at this more closely and I guess it's correct. Please wait for @chandlerc to sign-off, though.

This revision is now accepted and ready to land.Dec 22 2016, 9:29 AM
This revision was automatically updated to reflect the committed changes.
sepavloff marked 2 inline comments as done.

Did @chandlerc approve?

No...

And I'm not a big fan. I don't understand why we can't make this specific to analyses, or whether it's really worthwhile if we can't...

I lost track of this. Please revert as Chandler raised some concerns about the approach. Please also consider to wait if you're asked to.

Reverted in r292062.
I am sorry for misunderstanding.