Page MenuHomePhabricator

[llc/opt] Add an option to run all passes twice
ClosedPublic

Authored by loladiro on Nov 24 2015, 1:27 PM.

Details

Summary

Lately, I have submitted a number of patches to fix bugs that only occurred when using the same pass manager to compile multiple modules (generally these bugs are failure to reset some persistent state). Unfortunately I don't think there is currently a way to test that from the command line. This adds a very simple flag to both llc and opt, under which the tools will simply re-run their respective pass pipelines using the same pass manager on the same module. I believe that should be sufficient to be able to test for that kind of bug, but any other ideas would be appreciated as well.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 41078.Nov 24 2015, 1:27 PM
loladiro retitled this revision from to [llc/opt] Add an option to run all passes twice.
loladiro updated this object.
loladiro set the repository for this revision to rL LLVM.
loladiro added subscribers: llvm-commits, kcc.

That's a great idea. Maybe add a LIT test that uses this flag so breakages will be caught at the buildbots?

The motivation for this were D14962 and D14963, so assuming this is an ok path forward, I was planning to just commit this first and add the appropriate tests there.

This makes sense.

Won't the second run output will overwrite the first run output?
If so, what do you think about renaming the second output so it may be binary compared to the first one?

I think what happens currently is that it will just append the second one
at the end of the first. I originally thought binary comparing the output
was not sensible as we're actually running over the same module twice and
the passes are of course not idempotent, but now I'm thinking maybe it's
better to clone the module and allow this. I'll take a look.

You are right, running passes over again for all but trivial code will not result in the same module.
The CloneModule approach should work.

loladiro updated this revision to Diff 41495.Dec 1 2015, 5:22 AM

Ok, how about doing this for the run-twice option:

  • Clone the module to make sure the operation is idempotent
  • Emit both results into buffers and always compare them to make sure they're equal
  • Always emit the result of the second run

That way, to investigate a failure here, run it once without the run-twice option,
once with the run twice option and compare the outputs.

I also added some trivial tests, just to make sure these are exercised now that
they actually have non-trivial behavior.

Do you see any reasonable way to share the duplicated code between llc.cpp and opt.cpp?

I don't see any existing abstraction that this naturally fits into, so I'm not sure. Did you have something in mind?

yaron.keren accepted this revision.Dec 3 2015, 1:35 PM
yaron.keren added a reviewer: yaron.keren.

Only abstracting the whole llc/opt drivers which is more trouble than this is worth.
Maybe a commment in llc that opt does something similar and vice versa.
LGTM.

This revision is now accepted and ready to land.Dec 3 2015, 1:35 PM
This revision was automatically updated to reflect the committed changes.
dschuff added a subscriber: dschuff.Dec 4 2015, 2:51 PM
dschuff added inline comments.
llvm/trunk/tools/opt/opt.cpp
623

This use of CloneModule means that llc now depends on TransformUtils (so it needed to be added to required_libraries in LLVMBuild.txt and CMakeLists.txt)

Fixed in r254786

Thanks! Sorry about that.