This is an archive of the discontinued LLVM Phabricator instance.

NewGVN: Fix PR 31501.
ClosedPublic

Authored by dberlin on Jan 4 2017, 1:30 PM.

Details

Summary

LLVM's non-standard notion of phi nodes means we can't both try to substitute for undef in phi nodes *and* use phi nodes as leaders all the time. This changes NewGVN to use the same semantics as SimplifyPHINode to decide which phi nodes are equivalent.

Event Timeline

dberlin updated this revision to Diff 83117.Jan 4 2017, 1:30 PM
dberlin retitled this revision from to NewGVN: Fix PR 31501..
dberlin updated this object.
dberlin added a reviewer: davide.
dberlin added a subscriber: llvm-commits.

Note: I 3-staged this change on both darwin, and linux.

(I will also split out the variable naming change, thought i already did)

Note: I 3-staged this change on both darwin, and linux.

How exactly did you manage to pass '-mllvm -enable-newgvn' to the stage1/stage2 compilers in stage2/stage3 without tickling https://llvm.org/bugs/show_bug.cgi?id=31506 on darwin?

Confirmed that the proposed patch builds a full 3-stage bootstrap with stage2/stage3 comparison on x86_64-apple-darwin16 for llvm/cfe/clang-tools-extra/polly/openmp/libcxx/compiler-rt when https://llvm.org/bugs/show_bug.cgi?id=31506 is worked around with...

Index: lib/Transforms/IPO/PassManagerBuilder.cpp
===================================================================
--- lib/Transforms/IPO/PassManagerBuilder.cpp	(revision 291142)
+++ lib/Transforms/IPO/PassManagerBuilder.cpp	(working copy)
@@ -71,7 +71,7 @@ static cl::opt<bool> RunLoadCombine("com
                                     cl::Hidden,
                                     cl::desc("Run the load combining pass"));
 
-static cl::opt<bool> RunNewGVN("enable-newgvn", cl::init(false), cl::Hidden,
+static cl::opt<bool> RunNewGVN("enable-newgvn", cl::init(true), cl::Hidden,
                                cl::desc("Run the NewGVN pass"));
 
 static cl::opt<bool>

instead of passing '-mllvm -enable-newgvn' on CXXFLAGS.

I"m going to commit this and let david post-commit review it for style, since we've verified it's correct, and it's clearly broken right now :)

Closed by commit rL291308: NewGVN: Fix PR 31501. (authored by dannyb). · Explain WhyJan 6 2017, 4:12 PM
This revision was automatically updated to reflect the committed changes.

Perhaps also flip the default on the RunNewGVN enable-newgvn opt to true to see if any remaining latent regressions can be smoked out on less common targets?

davide added a comment.Jan 6 2017, 5:52 PM

Perhaps also flip the default on the RunNewGVN enable-newgvn opt to true to see if any remaining latent regressions can be smoked out on less common targets?

We're nowhere near there yet, unfortunately.

davide added a comment.Jan 6 2017, 6:04 PM

Post commit review, the general idea is obviously correct but few comments

llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
393–396 ↗(On Diff #83456)

The rename is NFC, but, hey, whatever.

813–828 ↗(On Diff #83456)

Somehow duplicating the logic here is not ideal. I don't expect the semantic of SimplifyPHINode to change anytime soon, but in case it would, we're in a situation where we should keep the two copies in sync.
I wasn't able to find a nice way to share code between GVN and InstSimplify, so this looks good as is.

852 ↗(On Diff #83456)

auto *

1870 ↗(On Diff #83456)

Any reason why you removed this newline?

1998–1999 ↗(On Diff #83456)

Why this assertion doesn't hold anymore?

dberlin marked 3 inline comments as done.Jan 6 2017, 7:31 PM
dberlin added inline comments.
llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
1870 ↗(On Diff #83456)

fixed

1998–1999 ↗(On Diff #83456)
if (isa<ConstantExpression>(E))
      assert(isa<Constant>(EClass->RepLeader) &&
             "Any class with a constant expression should have a "
             "constant leader");

verifies it now during congruence finding

davide added a comment.Jan 6 2017, 7:33 PM

LGTM again, thanks for taking care of the post commit comments.

llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
1998–1999 ↗(On Diff #83456)

Cool! I missed it :(

Perhaps also flip the default on the RunNewGVN enable-newgvn opt to true to see if any remaining latent regressions can be smoked out on less common targets?

We're nowhere near there yet, unfortunately.

Enabling NewGVN here produces only 11 regressions in the llvm test suite on x86_64-apple-darwin16...

Failing Tests (11):

  LLVM :: BugPoint/compile-custom.ll
  LLVM :: BugPoint/crash-narrowfunctiontest.ll
  LLVM :: BugPoint/invalid-debuginfo.ll
  LLVM :: BugPoint/metadata.ll
  LLVM :: BugPoint/named-md.ll
  LLVM :: BugPoint/remove_arguments_test.ll
  LLVM :: BugPoint/replace-funcs-with-null.ll
  LLVM :: Transforms/Coroutines/ArgAddr.ll
  LLVM :: Transforms/Coroutines/ex5.ll
  LLVM :: Transforms/GVN/no_speculative_loads_with_asan.ll
  LLVM :: Transforms/NewGVN/no_speculative_loads_with_asan.ll
 
Expected Passes    : 14131
Expected Failures  : 80
Unsupported Tests  : 4996
Unexpected Failures: 11
jwhowarth added a comment.EditedJan 6 2017, 8:57 PM
The most interesting failures are those of the format...

/sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./bin/bugpoint -load /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./lib/BugpointPasses.dylib /sw/src/fink.build/llvm40-4.0.0-1/llvm-4.0.0.src/test/BugPoint/remove_arguments_test.ll -output-prefix /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/test/BugPoint/Output/remove_arguments_test.ll.tmp -bugpoint-crashcalls -silence-passes
/sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./bin/llvm-dis /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/test/BugPoint/Output/remove_arguments_test.ll.tmp-reduced-simplified.bc -o - | /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./bin/FileCheck /sw/src/fink.build/llvm40-4.0.0-1/llvm-4.0.0.src/test/BugPoint/remove_arguments_test.ll
--
Exit Code: 139

Command Output (stderr):
--
Two passes with the same argument (-domtree) attempted to be registered!
0  libLLVM.dylib            0x000000010733cb78 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40
1  libLLVM.dylib            0x000000010733d1f6 SignalHandler(int) + 566
2  libsystem_platform.dylib 0x00007fffdc343bba _sigtramp + 26
3  libsystem_platform.dylib 0x0000000114507c54 _sigtramp + 941375668
Stack dump:
0.	Program arguments: /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./bin/bugpoint -load /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./lib/BugpointPasses.dylib /sw/src/fink.build/llvm40-4.0.0-1/llvm-4.0.0.src/test/BugPoint/remove_arguments_test.ll -output-prefix /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/test/BugPoint/Output/remove_arguments_test.ll.tmp -bugpoint-crashcalls -silence-passes 
/sw/src/fink.build/llvm40-4.0.0-1/build/stage3/test/BugPoint/Output/remove_arguments_test.ll.script: line 2:  3748 Segmentation fault: 11  /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./bin/bugpoint -load /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/./lib/BugpointPasses.dylib /sw/src/fink.build/llvm40-4.0.0-1/llvm-4.0.0.src/test/BugPoint/remove_arguments_test.ll -output-prefix /sw/src/fink.build/llvm40-4.0.0-1/build/stage3/test/BugPoint/Output/remove_arguments_test.ll.tmp -bugpoint-crashcalls -silence-passes