[llvm] r300200 - Re-apply "[GVNHoist] Move GVNHoist to function simplification part of pipeline."

[Geoff Berry via llvm-commits]

If any one decides to revert this, I'd like to request that you attempt 
to do so by just turning off GVNHoist by default instead of reverting 
r300200 if possible.  I believe the current location in the pass 
pipeline is the desired one, and reverting r300200 will just move it 
back to being done before inlining, which is likely just hiding bugs.

FWIW, before re-enabling this most recently I did correctness testing on 
X86 and AArch64 of the test-suite, SPEC and several other benchmarks, as 
well as bootstrapping clang, all without issue.

On 4/23/2017 12:45 PM, Philip Reames wrote:
> If this does turn out to be the problematic commit, I'll ask that we 
> do not re-enable GVNHoist without some llvm-dev discussion of what 
> we've done to test it. I've literally lost track of the number of 
> times this has been enabled and disabled by now.  There's clearly a 
> problem here; it's time we discuss it and what needs done to address 
> it going forward.
>
> Philip
>
> On 04/21/2017 08:24 PM, Chandler Carruth via llvm-commits wrote:
>> FYI and just as a heads up, we're seeing buggy hoisting leading to 
>> null pointers being accessed, and this commit is in the range and 
>> seems very likely to be responsible. We're working on getting this 
>> isolated enough to be sure, but wanted to mention it in case anyone 
>> else is similarly looking at issues after this commit.
>>
>> On Thu, Apr 13, 2017 at 8:49 AM Geoff Berry via llvm-commits 
>> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>>     Author: gberry
>>     Date: Thu Apr 13 10:36:25 2017
>>     New Revision: 300200
>>
>>     URL: http://llvm.org/viewvc/llvm-project?rev=300200&view=rev
>>     Log:
>>     Re-apply "[GVNHoist] Move GVNHoist to function simplification
>>     part of pipeline."
>>
>>     This reverts commit r296872 now that PR32153 has been fixed.
>>
>>     Added:
>>         llvm/trunk/test/Transforms/GVNHoist/hoist-inline.ll
>>     Modified:
>>         llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp
>>
>>     Modified: llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp
>>     URL:
>>     http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp?rev=300200&r1=300199&r2=300200&view=diff
>>     ==============================================================================
>>     --- llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp (original)
>>     +++ llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp Thu Apr
>>     13 10:36:25 2017
>>     @@ -245,8 +245,6 @@ void PassManagerBuilder::populateFunctio
>>        FPM.add(createCFGSimplificationPass());
>>        FPM.add(createSROAPass());
>>        FPM.add(createEarlyCSEPass());
>>     -  if (EnableGVNHoist)
>>     -    FPM.add(createGVNHoistPass());
>>        FPM.add(createLowerExpectIntrinsicPass());
>>      }
>>
>>     @@ -291,6 +289,8 @@ void PassManagerBuilder::addFunctionSimp
>>        // Break up aggregate allocas, using SSAUpdater.
>>        MPM.add(createSROAPass());
>>        MPM.add(createEarlyCSEPass());              // Catch trivial
>>     redundancies
>>     +  if (EnableGVNHoist)
>>     +    MPM.add(createGVNHoistPass());
>>        // Speculative execution if the target has divergent branches;
>>     otherwise nop.
>>      MPM.add(createSpeculativeExecutionIfHasBranchDivergencePass());
>>        MPM.add(createJumpThreadingPass());         // Thread jumps.
>>
>>     Added: llvm/trunk/test/Transforms/GVNHoist/hoist-inline.ll
>>     URL:
>>     http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVNHoist/hoist-inline.ll?rev=300200&view=auto
>>     ==============================================================================
>>     --- llvm/trunk/test/Transforms/GVNHoist/hoist-inline.ll (added)
>>     +++ llvm/trunk/test/Transforms/GVNHoist/hoist-inline.ll Thu Apr
>>     13 10:36:25 2017
>>     @@ -0,0 +1,38 @@
>>     +; RUN: opt -S -O2 < %s | FileCheck %s
>>     +
>>     +; Check that the inlined loads are hoisted.
>>     +; CHECK-LABEL: define i32 @fun(
>>     +; CHECK-LABEL: entry:
>>     +; CHECK: load i32, i32* @A
>>     +; CHECK: if.then:
>>     +
>>     + at A = external global i32
>>     + at B = external global i32
>>     + at C = external global i32
>>     +
>>     +define i32 @loadA() {
>>     +   %a = load i32, i32* @A
>>     +   ret i32 %a
>>     +}
>>     +
>>     +define i32 @fun(i1 %c) {
>>     +entry:
>>     +  br i1 %c, label %if.then, label %if.else
>>     +
>>     +if.then:
>>     +  store i32 1, i32* @B
>>     +  %call1 = call i32 @loadA()
>>     +  store i32 2, i32* @C
>>     +  br label %if.endif
>>     +
>>     +if.else:
>>     +  store i32 2, i32* @C
>>     +  %call2 = call i32 @loadA()
>>     +  store i32 1, i32* @B
>>     +  br label %if.endif
>>     +
>>     +if.endif:
>>     +  %ret = phi i32 [ %call1, %if.then ], [ %call2, %if.else ]
>>     +  ret i32 %ret
>>     +}
>>     +
>>
>>
>>     _______________________________________________
>>     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
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>

-- 
Geoff Berry
Employee of Qualcomm Datacenter Technologies, Inc.
  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170423/3b4770bc/attachment.html>