[llvm] r233153 - 'optnone' should not disable DAG combiner.

[Robinson, Paul]

you could phrase the test in terms of comparing optnone and non-optnone behavior, ensuring they produce identical answers?

For optnone tests in general, comparing non-O0 optnone and –O0 non-optnone is what we want.  I don't see a way to persuade llc to apply different attributes to a function based on RUN line parameters, so the naïve run-twice-and-diff approach doesn't seem applicable. So, existing optnone tests compile functions with optnone and look for particular instructions.

In this specific case, it's complexified by needing an O0 case that bails out of fast-isel and then triggers a DAG combine, which is what the test would actually be looking for.  Thus, the test inherently depends on having some situation where fast-isel will bail out.  I know a way to make that work, but if somebody wants to provide a more robust tactic for writing a test that bails out of fast-isel, I'm happy to adopt it.
--paulr

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Wednesday, March 25, 2015 10:34 AM
To: Robinson, Paul
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r233153 - 'optnone' should not disable DAG combiner.



On Wed, Mar 25, 2015 at 8:46 AM, Robinson, Paul <Paul_Robinson at playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>> wrote:

Leave the test case in and switch the expectation (& perhaps add a comment explaining why this the right choice and the previous one wasn't) so there's breadcrumbs if anyone else gets the same idea again in the future?
Hmmmm the conditions where the test failed were pretty obscure and we used a sledgehammer to drive a finishing nail. But I could see a way to make the test prove that if an optnone function bails out of fast-isel, the DAG combiner does run, which would be the point you're driving at.  It'd get a false failure if fast-isel got smarter but with the right commentary that ought to be okay.  Will work on this later today.

To keep the test non-brittle (though perhaps it'd become pointless, rather than a false failure) you could phrase the test in terms of comparing optnone and non-optnone behavior, ensuring they produce identical answers? Maybe - perhaps that's not a good idea, I'm not sure.

- David

--paulr

From: David Blaikie [mailto:dblaikie at gmail.com<mailto:dblaikie at gmail.com>]
Sent: Tuesday, March 24, 2015 7:13 PM
To: Robinson, Paul
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: Re: [llvm] r233153 - 'optnone' should not disable DAG combiner.


On Mar 24, 2015 5:18 PM, "Paul Robinson" <paul_robinson at playstation.sony.com<mailto:paul_robinson at playstation.sony.com>> wrote:
>
> Author: probinson
> Date: Tue Mar 24 19:10:24 2015
> New Revision: 233153
>
> URL: http://llvm.org/viewvc/llvm-project?rev=233153&view=rev
> Log:
> 'optnone' should not disable DAG combiner.
>
> Reverts the code change from r221168 and the relevant test.
> It was a mistake to disable the combiner, and based on the ultimate
> definition of 'optnone' we shouldn't have considered the test case
> as failing in the first place.

Leave the test case in and switch the expectation (& perhaps add a comment explaining why this the right choice and the previous one wasn't) so there's breadcrumbs if anyone else gets the same idea again in the future?

>
> Removed:
>     llvm/trunk/test/CodeGen/X86/fastmath-optnone.ll
> Modified:
>     llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=233153&r1=233152&r2=233153&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Mar 24 19:10:24 2015
> @@ -1183,11 +1183,6 @@ void DAGCombiner::Run(CombineLevel AtLev
>    LegalOperations = Level >= AfterLegalizeVectorOps;
>    LegalTypes = Level >= AfterLegalizeTypes;
>
> -  // Early exit if this basic block is in an optnone function.
> -  if (DAG.getMachineFunction().getFunction()->hasFnAttribute(
> -          Attribute::OptimizeNone))
> -    return;
> -
>    // Add all the dag nodes to the worklist.
>    for (SelectionDAG::allnodes_iterator I = DAG.allnodes_begin(),
>         E = DAG.allnodes_end(); I != E; ++I)
>
> Removed: llvm/trunk/test/CodeGen/X86/fastmath-optnone.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fastmath-optnone.ll?rev=233152&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/fastmath-optnone.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/fastmath-optnone.ll (removed)
> @@ -1,35 +0,0 @@
> -; RUN: llc < %s -mcpu=corei7 -march=x86-64 -mattr=+sse2 | FileCheck %s
> -; Verify that floating-point operations inside 'optnone' functions
> -; are not optimized even if unsafe-fp-math is set.
> -
> -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-NOT: add
> -; CHECK: 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: mul
> -; CHECK: add
> -; CHECK-NOT: mul
> -; CHECK: add
> -; CHECK-NOT: mul
> -; CHECK: ret
> -
> -
> -attributes #0 = { "unsafe-fp-math"="true" }
> -attributes #1 = { noinline optnone "unsafe-fp-math"="true" }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150325/7926c359/attachment.html>