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

8

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

8

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 -O0 -mattr=+avx | FileCheck %s
+
+; Background:
+; If fast-isel bails out to normal selection, then the DAG combiner will run,
+; even at -O0. In principle this should not happen (those are optimizations,
+; and we said -O0) but as a practical matter there are some instruction
+; selection patterns that depend on the legalizations and transforms that the
+; DAG combiner does.
+;
+; The 'optnone' attribute implicitly sets -O0 and fast-isel for the function.
+; The DAG combiner was disabled for 'optnone' (but not -O0) by r221168, then
+; re-enabled in r233153 because of problems with instruction selection patterns
+; mentioned above. (Note: because 'optnone' is supposed to match -O0, r221168
+; really should have disabled the combiner for both.)
+;
+; If instruction selection eventually becomes smart enough to run without DAG
+; combiner, then the combiner can be turned off for -O0 (not just 'optnone')
+; and this test can go away. (To be replaced by a different test that verifies
+; the DAG combiner does *not* run at -O0 or for 'optnone' functions.)
+;
+; In the meantime, this test wants to make sure the combiner stays enabled for
+; 'optnone' functions, just as it is for -O0.
+
+
+; The test cases @foo[WithOptnone] prove that the same DAG combine happens
+; with -O0 and with 'optnone' set.  To prove this, we use a Windows triple to
+; cause fast-isel to bail out (because something about the calling convention
+; is not handled in fast-isel).  Then we have a repeated fadd that can be
+; combined into an fmul.  We show that this happens in both the non-optnone
+; function and the optnone function.
+
+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
+
+
+; The test case @bar is derived from an instruction selection failure case
+; that was solved by r233153. It depends on -mattr=+avx.
+; Really all we're trying to prove is that it doesn't crash any more.
+
+@id84 = common global <16 x i32> zeroinitializer, align 64
+
+define void @bar() #1 {
+entry:
+  %id83 = alloca <16 x i8>, align 16
+  %0 = load <16 x i32>, <16 x i32>* @id84, align 64
+  %conv = trunc <16 x i32> %0 to <16 x i8>
+  store <16 x i8> %conv, <16 x i8>* %id83, align 16
+  ret void
+}
+
+attributes #0 = { "unsafe-fp-math"="true" }
+attributes #1 = { noinline optnone "unsafe-fp-math"="true" }