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

[Kaylor, Andrew via llvm-commits]

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?)

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<mailto: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<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160708/7b116982/attachment-0001.html>