This is an archive of the discontinued LLVM Phabricator instance.

Verify 'optnone' can run DAG combiner when appropriate
ClosedPublic

Authored by probinson on Mar 25 2015, 1:17 PM.

Details

Summary

DAG combiner had been disabled for 'optnone' functions in r221168.
It was re-enabled in r233153. Add a test to keep it from happening again.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 22669.Mar 25 2015, 1:17 PM
probinson retitled this revision from to Verify 'optnone' can run DAG combiner when appropriate.
probinson updated this object.
probinson edited the test plan for this revision. (Show Details)
probinson added a reviewer: dblaikie.
probinson added a subscriber: Unknown Object (MLST).

Inline comment.

test/CodeGen/X86/dag-optnone.ll
7 ↗(On Diff #22669)

Could you describe the problem that would occur were the dag combiner not run? That hasn't been clear either in this test or in your original commit reverting.

probinson added inline comments.Mar 25 2015, 2:25 PM
test/CodeGen/X86/dag-optnone.ll
7 ↗(On Diff #22669)

The most basic description of 'optnone' is "be as much like -O0 as practical." The patch that I reverted would disable DAG combine specifically for 'optnone' but not for -O0. This caused spurious differences between -O0 and 'optnone' which were caught by some internal tests designed to expect no differences.
You might also remember back in January, I had a patch to turn off the MergeConsecutiveStores combine at -O0 (and therefore also for 'optnone'), see D7181. The question came up then (by you and Jim Grosbach) that DAG combine shouldn't run at all, at -O0. But other people disagreed, and in fact turning it off at -O0 can cause instruction-selection failures. So, instead of turning off all DAG combines for -O0, I did just the one combine.
That left DAG Combiner turned off for 'optnone' but not -O0, which is both abstractly inconsistent and causes that same variety of instruction-selection failures if you tag the right functions as 'optnone' instead of compiling at -O0. (We have an internal fuzzer that ran into this problem as well.) So, I took out the optnone-only patch, restoring balance to the optnone/O0 equation. Our internal tests are all happy again.
Blaikie then wanted an LLVM test that made sure we didn't make that mistake again. The IR for this test is exactly the same as the test I had removed; I changed the RUN line to invoke the conditions that would cause fast-isel to bail out, and changed the CHECK lines to verify that the DAG combine had run.

probinson updated this revision to Diff 22682.Mar 25 2015, 3:15 PM

Add a bit more detail to the comment explaining the test.
Run llc at -O0 to show we expect the combine at -O0, and also see it with optnone.

"What caused you to notice not running the DAG combiner was a problem?"

A fuzzer that we wrote generated some functions with 'optnone' and they crashed the compiler with instruction-selection failures.
Being mostly vector-related, Andrea looked at them, and didn't take long to point the finger at DAG combine (or lack thereof!).

Should I take this to imply you'd rather have one of the causes-a-crash functions as the test, instead of this one?
--paulr

From: Eric Christopher [mailto:echristo@gmail.com]
Sent: Wednesday, March 25, 2015 4:44 PM
To: reviews+D8614+public+1ca59dfb9fe35dfd@reviews.llvm.org; Robinson, Paul; dblaikie@gmail.com
Cc: llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] Verify 'optnone' can run DAG combiner when appropriate

probinson updated this revision to Diff 22837.Mar 27 2015, 5:17 PM

Add an extensive background description to the commentary, with particular attention to sources of possible fragility in the test.

incorporate a test case that used to fail instruction selection.
Yes, it's a this-doesn't-crash test, but I really have no clue what it could robustly check for; the vector stuff is seeing a whole lot of churn lately, and I don't understand the vector instructions in any useful way in the first place. If somebody can make a specific suggestion, I'm happy to add something.

echristo accepted this revision.Mar 30 2015, 10:44 AM
echristo added a reviewer: echristo.

Looks good. FWIW this is what should have been in the revert patch commit message as well :)

-eric

This revision is now accepted and ready to land.Mar 30 2015, 10:44 AM
This revision was automatically updated to reflect the committed changes.

Looks good. FWIW this is what should have been in the revert patch commit message as well :)

-eric

FWIW my thinking was "undo the obvious mistake." Just not as obvious as I thought, I guess!

And although Phab has recorded the revision correctly on the web page, it's not in the email, so:
r233584.