[llvm] r274786 - Include SelectionDAGISel in the opt-bisect process

Thu Jul 7 19:47:49 PDT 2016

----- Original Message -----

> From: "Andrew Kaylor" <andrew.kaylor at intel.com>
> To: "Michael Kuperstein" <mkuper at google.com>, hfinkel at anl.gov
> Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> Sent: Thursday, July 7, 2016 9:34:42 PM
> Subject: RE: [llvm] r274786 - Include SelectionDAGISel in the
> opt-bisect process

> Hi Michael,

> The reason my changes broke the PPC buildbots is that the
> SelectionDAGISel is always run at CodeGenOpt::Default for PPC
> targets, even if -O0 has been specified on the command line. My
> change added a check for the OptNone function attribute and the
> opt-bisect-limit value in the SelectionDAGISel base class. There
> were tests in place to verify that no passes are skipped based on
> either of these options that are otherwise run at -O0. With my
> change the pass isn't actually skipped, but it behaves differently
> (doesn't optimize anything) and so it is reported as being skipped.
> I had added a check to see if we started at CodeGenOpt::None to
> avoid false reporting, but since the OptLevel is never set to
> CodeGenOpt::None for PPC targets the "skip” was reported and the
> tests failed.

> Looking at the code, I expect that AMDGPU, BPF, Mips, and Sparc would
> all fail for the same reason.

> So it turns out that the fix for this isn't entirely simple. I
> removed the test cases that were failing, but they really should be
> put back soon. That leaves the problem of how to avoid the failure
> for the platforms that aren't respecting the global CodeGenOpt level
> for SelectionDAGISel.

> It seems to me that all platforms should be handling CodeGenOpt
> levels correctly. There's a default value in the SelectionDAGISel
> constructor which leads to -O2 behavior if no other value is
> explicitly passed in. I think that's a bad idea.

> I could arguably work around the test failure by suppressing the skip
> message in some circumstances or modifying the test to tolerate this
> one pass being skipped, but that feels like a step in the wrong
> direction. Ultimately, the test failure led to an awareness that
> this pass was performing optimizations when optimizations should
> have been disabled and that seems like a good thing. The intention
> of the test was to alert us to passes being skipped that shouldn't
> have been skipped, but it seems that it can also help point to
> passes performing optimizations when they shouldn't be doing so.

> My inclination at this point is to reinstate the failing test cases
> and mark this test as XFAIL for the platforms that aren't handling
> opt level for SelectionDAGISel. That's less than ideal, but I think
> it's the best short-term solution available at the moment.

> There are six PowerPC tests which fail it the
> SelectionDAGISel::OptLevel member is set to CodeGenOpt::None when
> TM->getOptLevel() returns CodeGenOpt::None, but I don’t know if the
> failing cases are testing required behavior or were just written to
> match actual output.

> (I'm CC'ing Hal Finkel for his opinion on the PowerPC behavior. Hal,
> can you drop me a note so I know you saw this?)
I assume that the tests are probably just matching whatever is the current output. I can't think of any reason why we'd need any particular isel optimizations for correctness, but obviously I'd need to look at what's happening to be sure. Can you please send the patch which fixes this (minus test-case changes)? I, or one of the other PowerPC-backend developers, will fix up the test cases, etc. 

Once all of the targets are fixed, removing the constructor's default argument seems reasonable. 

-Hal 

> Thanks,
> Andy

> From: Michael Kuperstein [mailto:mkuper at google.com]
> Sent: Thursday, July 07, 2016 4:38 PM
> To: Kaylor, Andrew <andrew.kaylor at intel.com>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>
> Subject: Re: [llvm] r274786 - Include SelectionDAGISel in the
> opt-bisect process

> Andrew, I think this broke the PPC bots. Could you please take a
> look?

> Thanks,

> Michael

> On Thu, Jul 7, 2016 at 11:55 AM, Andrew Kaylor via llvm-commits <
> llvm-commits at lists.llvm.org > wrote:
> > Author: akaylor
> 
> > Date: Thu Jul 7 13:55:02 2016
> 
> > New Revision: 274786
> 

> > URL: http://llvm.org/viewvc/llvm-project?rev=274786&view=rev
> 
> > Log:
> 
> > Include SelectionDAGISel in the opt-bisect process
> 

> > Differential Revision: http://reviews.llvm.org/D21143
> 

> > Added:
> 
> > llvm/trunk/test/Other/X86/opt-bisect-isel.ll
> 
> > Modified:
> 
> > llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> 

> > Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> 
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp?rev=274786&r1=274785&r2=274786&view=diff
> 
> > ==============================================================================
> 
> > --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> > (original)
> 
> > +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp Thu
> > Jul
> > 7 13:55:02 2016
> 
> > @@ -443,7 +443,7 @@ bool SelectionDAGISel::runOnMachineFunct
> 
> > TM.resetTargetOptions(Fn);
> 
> > // Reset OptLevel to None for optnone functions.
> 
> > CodeGenOpt::Level NewOptLevel = OptLevel;
> 
> > - if (Fn.hasFnAttribute(Attribute::OptimizeNone))
> 
> > + if (OptLevel != CodeGenOpt::None && skipFunction(Fn))
> 
> > NewOptLevel = CodeGenOpt::None;
> 
> > OptLevelChanger OLC(*this, NewOptLevel);
> 

> > Added: llvm/trunk/test/Other/X86/opt-bisect-isel.ll
> 
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/X86/opt-bisect-isel.ll?rev=274786&view=auto
> 
> > ==============================================================================
> 
> > --- llvm/trunk/test/Other/X86/opt-bisect-isel.ll (added)
> 
> > +++ llvm/trunk/test/Other/X86/opt-bisect-isel.ll Thu Jul 7 13:55:02
> > 2016
> 
> > @@ -0,0 +1,22 @@
> 
> > +; This test verifies that no optimizations are performed on the @f
> > function
> 
> > +; when the -opt-bisect-limit=0 option is used. In particular, the
> > X86
> 
> > +; instruction selector will optimize the cmp instruction to a sub
> > instruction
> 
> > +; if it is not run in -O0 mode.
> 
> > +
> 
> > +; RUN: llc -O3 -opt-bisect-limit=0 -o - %s | FileCheck %s
> 
> > +
> 
> > +target triple = "x86_64-unknown-linux-gnu"
> 
> > +
> 
> > +define void @f() {
> 
> > +entry:
> 
> > + %cmp = icmp slt i32 undef, 8
> 
> > + br i1 %cmp, label %middle, label %end
> 
> > +
> 
> > +middle:
> 
> > + br label %end
> 
> > +
> 
> > +end:
> 
> > + ret void
> 
> > +}
> 
> > +
> 
> > +; CHECK: cmpl $8, %eax
> 

> > _______________________________________________
> 
> > llvm-commits mailing list
> 
> > llvm-commits at lists.llvm.org
> 
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160707/b563e259/attachment.html>