[llvm] r338794 - [LICM] Remove unneccessary safety check to increase sinking effectiveness

[Philip Reames via llvm-commits]

Author: reames
Date: Thu Aug  2 17:21:56 2018
New Revision: 338794

URL: http://llvm.org/viewvc/llvm-project?rev=338794&view=rev
Log:
[LICM] Remove unneccessary safety check to increase sinking effectiveness

This one requires a bit of explaination.  It's not every day you simply delete code to implement an optimization.  :)

The transform in question is sinking an instruction from a loop to the uses in loop exiting blocks.  We know (from LCSSA) that all of the uses outside the loop must be phi nodes, and after predecessor splitting, we know all phi users must have a single operand.  Since the use must be strictly dominated by the def, we know from the definition of dominance/ssa that the exit block must execute along a (non-strict) subset of paths which reach the def.  As a result, duplicating a potentially faulting instruction can not *introduce* a fault that didn't previously exist in the program.  

The full story is that this patch builds on "rL338671: [LICM] Factor out fault legality from canHoistOrSinkInst [NFC]" which pulled this logic out of a common helper routine.  As best I can tell, this check was originally added to the helper function for hoisting legality, later an incorrect fastpath for loads/calls was added, and then the bug was fixed by duplicating the fault safety check in the hoist path.  This left the redundant check in the common code to pessimize sinking for no reason.  I split it out in an NFC, and am not removing the unneccessary check.  I wanted there to be something easy to revert in case I missed something.

Reviewed by: Anna Thomas (in person)


Modified:
    llvm/trunk/lib/Transforms/Scalar/LICM.cpp
    llvm/trunk/test/Transforms/LICM/sinking.ll

Modified: llvm/trunk/lib/Transforms/Scalar/LICM.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LICM.cpp?rev=338794&r1=338793&r2=338794&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/LICM.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/LICM.cpp Thu Aug  2 17:21:56 2018
@@ -412,14 +412,7 @@ bool llvm::sinkRegion(DomTreeNode *N, Al
       //
       bool FreeInLoop = false;
       if (isNotUsedOrFreeInLoop(I, CurLoop, SafetyInfo, TTI, FreeInLoop) &&
-          canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, true, ORE) &&
-          // FIXME: Remove the special casing here.  Why do we need the
-          // execution check at all?  We're sinking from a dominating location
-          // to a dominated location.
-          (isa<LoadInst>(I) || isa<CallInst>(I) ||
-           // TODO: Plumb the context instruction through to make sinking
-           // more powerful. 
-           isSafeToExecuteUnconditionally(I, DT, CurLoop, SafetyInfo, ORE))) {
+          canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, true, ORE)) {
         if (sink(I, LI, DT, CurLoop, SafetyInfo, ORE, FreeInLoop)) {
           if (!FreeInLoop) {
             ++II;

Modified: llvm/trunk/test/Transforms/LICM/sinking.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LICM/sinking.ll?rev=338794&r1=338793&r2=338794&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/LICM/sinking.ll (original)
+++ llvm/trunk/test/Transforms/LICM/sinking.ll Thu Aug  2 17:21:56 2018
@@ -53,8 +53,8 @@ Out:		; preds = %Loop
 	ret i32 %A
 ; CHECK-LABEL: @test2b(
 ; CHECK: Out:
-; CHECK-NOT: sdiv
-; CHECK: ret i32 %A.lcssa
+; CHECK-NEXT: sdiv
+; CHECK-NEXT: ret i32 %A
 }
 
 define double @test2c(double* %P) {