Verify 'optnone' can run DAG combiner when appropriate

[probinson]

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.

[echristo]

Inline comment.

test/CodeGen/X86/dag-optnone.ll

7

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]

test/CodeGen/X86/dag-optnone.ll

7

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]

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.

[probinson]

"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:[e-mail]]
Sent: Wednesday, March 25, 2015 4:44 PM
To: [e-mail]; Robinson, Paul; [e-mail]
Cc: [e-mail]
Subject: Re: [PATCH] Verify 'optnone' can run DAG combiner when appropriate

[probinson]

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]

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

-eric

[probinson]

In D8614#149007, @echristo wrote:

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!

[probinson]

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

test/CodeGen/X86/dag-optnone.ll

+; RUN: llc < %s -mtriple=x86_64-pc-win32 | FileCheck %s
+; Verify that when an 'optnone' function bails out of fast-isel, it will run
+; the DAG combiner. (This was disabled by r221168, re-enabled in r233153.)
+; We set unsafe-fp-math which allows the DAG combiner to fold the fadd sequence
+; into an fmul. We have a non-optnone function doing the same thing, to show
+; that the combine is expected.
+; Note, this will start failing if somebody decides that this combine should
+; not occur at -O0 (because 'optnone' essentially sets -O0 for the function).
+; FIXME: We use a Windows triple because the calling convention ends up causing
+; fast-isel to bail out.  A more robust tactic for ensuring fast-isel will
+; bail out would be better.
+
+define float @foo(float %x) #0 {
+entry:
+  %add = fadd fast float %x, %x
+  %add1 = fadd fast float %add, %x
+  ret float %add1
+}
+
+; CHECK-LABEL: @foo
+; CHECK-NOT:   add
+; CHECK:       mul
+; CHECK-NEXT:  ret
+
+define float @fooWithOptnone(float %x) #1 {
+entry:
+  %add = fadd fast float %x, %x
+  %add1 = fadd fast float %add, %x
+  ret float %add1
+}
+
+; CHECK-LABEL: @fooWithOptnone
+; CHECK-NOT:   add
+; CHECK:       mul
+; CHECK-NEXT:  ret
+
+attributes #0 = { "unsafe-fp-math"="true" }
+attributes #1 = { noinline optnone "unsafe-fp-math"="true" }