This is an archive of the discontinued LLVM Phabricator instance.

llvm-reduce: reduce using opt passes
AbandonedPublic

Authored by regehr on Jun 23 2022, 3:06 PM.

Details

Summary

This patch teaches llvm-reduce to run opt passes. The rationale for this comes in three parts:

  1. opt passes have the potential to usefully shrink test cases in ways that are more powerful than the existing delta passes that we have. for example, SROA cleans up a lot of allocas, InstCombine does lots of other simplifications, etc.
  2. running opt passes can bring the test case closer to the actual input that gets miscompiled. consider an example where we're reducing some unoptimized IR that gets miscompiled 25 passes into the phase ordering. by running some of these earlier passes, we make the test case into something that is more directly miscompiled, making it easier to understand what's going on and also making it easier to tell if the bug trigger is a duplicate.
  3. opt passes try to canonicalize the code, and we like canonicalization because for those of us who read a lot of canonical IR it's easier to understand. if there are non-canonical aspects to our test case that are inessential for triggering a bug, we should eliminate them.

A drawback of this patch is that it makes llvm-reduce run slower -- on my machine, this patch makes my machine take 2.4 seconds instead of 1.6 seconds to run all of the llvm-reduce test cases. The other drawback that I can think of is that if we're trying to reduce a file that crashes one of the passes we're trying to run, llvm-reduce will also crash (I have a solution to this, but was hoping to land this patch first).

One thing to discuss is the set of passes that we run here, I've just gone and picked some by hand that seem like they are mostly going to simplify the code, but am open to discussion about which passes make the most sense.

Another thing we could discuss is when to run the opt passes. Here I have them running right at the beginning, when there should be lots of opportunities for them to clean things up, but we could also make an argument for running them at the end of the llvm-reduce phase ordering so that they'll canonicalize the IR as much as possible before it gets dumped out for human consumption.

Diff Detail

Event Timeline

regehr created this revision.Jun 23 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 3:06 PM
regehr requested review of this revision.Jun 23 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 3:06 PM

this patch is currently failing some tests but I can fix that up quickly. I just benchmarked it and it reduces final file size by 7% on my test cases. not earthshattering, but the reduced files end up being somewhat more pleasant to look at, since the passes leave things looking nice and canonical. anyone have time to look this over?

sorry, forgot to review this the first time around

do we really need this many passes? I'd think that just SROA/InstCombine/SimplifyCFG would be enough

llvm/test/tools/llvm-reduce/remove-operands-fp.ll
26

I'd actually prefer not to have internal as the canonical reduced output, it's more likely to get thrown away by accident and also increases the amount of text in the output. ditto with tail calls

llvm/tools/llvm-reduce/DeltaManager.cpp
55

we should run this after some of the other passes that remove lots of stuff quickly since this is fairly expensive

llvm/tools/llvm-reduce/deltas/ReduceUsingOpt.h
21

this isn't running opt, it's just running passes. something like reduceUsingIRPassesDeltaPass seems more appropriate

arsenm added a comment.Aug 4 2022, 3:40 PM

sorry, forgot to review this the first time around

do we really need this many passes? I'd think that just SROA/InstCombine/SimplifyCFG would be enough

I get a lot of value out of manually running InferAddressSpaces and GVN when reducing

regehr added a comment.Aug 4 2022, 3:46 PM

Overlap with D123749?

aieee!!! I wish I'd seen that earlier. I'm totally happy to go with that one instead of this it if looks better to y'all.

I agree my list of passes is overkill, and also that gvn sounds super useful to run (I have no objection to InferAddressSpaces)

regehr added a comment.Aug 4 2022, 4:08 PM

here's a proposal: if we decide to go forward with my patch instead of Arthur's, I'll do a bit of work to remove the passes one by one in order to help us make a more informed decision about which passes we should be running here.

I'm not saying we should include every pass that reduces size, but rather that that data is a better starting point for this discussion than our intuition is.

regehr abandoned this revision.Aug 4 2022, 7:28 PM
arsenm added a comment.Aug 4 2022, 7:32 PM

here's a proposal: if we decide to go forward with my patch instead of Arthur's, I'll do a bit of work to remove the passes one by one in order to help us make a more informed decision about which passes we should be running here.

I'm not saying we should include every pass that reduces size, but rather that that data is a better starting point for this discussion than our intuition is.

I do think restricting this is more useful. Just running opt -O3 kills most failures I look at