Page MenuHomePhabricator

[RFC] re-enable GVNHoist by default
ClosedPublic

Authored by labrinea on Jul 26 2018, 8:30 AM.

Details

Summary

My initial motivation for this came from https://reviews.llvm.org/D48122, where it was pointed out that my change didn't fit well in SimplifyCFG and therefore using GVNHoist was a better way to go. GVNHoist has been disabled for a while as there was a list of bugs related to it.

I have fixed the following bugs:

The next two bugs no longer occur, and it's unclear which commit fixed them:

I investigated this one and proved to be unrelated to GVNHoist, but a genuine bug in NewGvn:

To convince myself GVNHoist is in a good state I made a successful bootstrap build of LLVM followed by check-all.

Diff Detail

Repository
rL LLVM

Event Timeline

labrinea created this revision.Jul 26 2018, 8:30 AM
xbolva00 added a subscriber: xbolva00.EditedJul 27 2018, 4:27 AM

Did you test it with some benchmarks? Results?

If no miscompiles and results are generally good, this should be enabled again (just in time for 7.0?). Successful bootstrap is a good sign.

What about that bug in newgvn? You wrote a detailed description of it, possible to fix it easily or it needs time?

sebpop accepted this revision.Jul 27 2018, 3:36 PM

Looks good to me.
Thanks Alexandros for fixing the last known bugs with gvn-hoist.

As Eli mentioned in https://reviews.llvm.org/D48122 the code of gvn-hoist has been used and tested quite extensively in the past year or two.
The initial implementation has been tested with llvm bootstrap, spec 2000, spec 2006, and several mobile proprietary benchmarks.

This revision is now accepted and ready to land.Jul 27 2018, 3:36 PM
xbolva00 accepted this revision.Jul 27 2018, 3:38 PM

If so, LG too

Did you test it with some benchmarks? Results?

I am running lnt, spec2000 and spec2006 on AArch64 at the moment. I'll post results soon.

What about that bug in newgvn? You wrote a detailed description of it, possible to fix it easily or it needs time?

I am not familiar with newgvn, therefore a fix will take time. Also it's out of the scope of this patch.

Since it is accepted, please merge it before Aug 1 (LLVM 7 branch time) :)

Closed by commit rL338240: [GVNHoist] Re-enable GVNHoist by default (authored by alelab01, committed by ). · Explain WhyJul 30 2018, 3:50 AM
This revision was automatically updated to reflect the committed changes.
labrinea reopened this revision.Aug 6 2018, 3:10 AM

This got reverted because of an out-of-memory error on an ubsan buildbot. Details and fix here -> https://reviews.llvm.org/D50323. I'll update the tests upon rebase.

This revision is now accepted and ready to land.Aug 6 2018, 3:10 AM
labrinea planned changes to this revision.Aug 6 2018, 3:11 AM
labrinea updated this revision to Diff 162843.Aug 28 2018, 6:28 AM

Rebase rL338240 since the excessive memory usage observed when using GVNHoist with UBSan has been fixed by rL340818 (https://reviews.llvm.org/D50323).

This revision is now accepted and ready to land.Aug 28 2018, 6:28 AM
labrinea requested review of this revision.Aug 28 2018, 6:29 AM
lebedev.ri added inline comments.
lib/Passes/PassBuilder.cpp
182 ↗(On Diff #162843)

Unrelated to this patch, but this desc is the same as for the previous option.
I suspect this should read "Enable the GVN sinking pass for the new PM (default = off)"

This revision is now accepted and ready to land.Aug 28 2018, 11:45 AM
Closed by commit rL340922: [GVNHoist] Re-enable GVNHoist by default (authored by alelab01, committed by ). · Explain WhyAug 29 2018, 4:59 AM
This revision was automatically updated to reflect the committed changes.