LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 32762 - Merge r298799 into the 4.0 branch : Split the SimplifyCFG pass into two variants.
Summary: Merge r298799 into the 4.0 branch : Split the SimplifyCFG pass into two varia...
Status: RESOLVED WONTFIX
Alias: None
Product: new-bugs
Classification: Unclassified
Component: new bugs (show other bugs)
Version: 4.0
Hardware: All All
: P enhancement
Assignee: Unassigned LLVM Bugs
URL: https://reviews.llvm.org/rL298799
Keywords:
Depends on:
Blocks: release-4.0.1
  Show dependency tree
 
Reported: 2017-04-23 14:29 PDT by Jörg Sonnenberger
Modified: 2017-05-24 03:11 PDT (History)
7 users (show)

See Also:
Fixed By Commit(s):

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jörg Sonnenberger 2017-04-23 14:29:14 PDT 
Is this patch OK to merge to the 4.0 branch?
Comment 1 Jörg Sonnenberger 2017-04-23 14:34:19 PDT 
Additionally, r298887 and r299558 for a problematic ARM/AArch64 test case.
Comment 2 Tom Stellard 2017-04-25 17:20:41 PDT 
This looks like it will break the ABI.  Also, this is a pretty big patch, does it fix a bug?
Comment 3 Renato Golin 2017-04-26 02:59:54 PDT 
It seems to be a performance improvement more than a bugfix. Even if there was a bug fixed in the process, I believe this one is just too big and core behavioural changing enough that it might not be a good candidate to a backport.

The two following revert/reapply also get me worried that there is an unstable behaviour that we'll be introducing...
Comment 4 Jörg Sonnenberger 2017-04-26 04:36:05 PDT 
It is not just a performance improvement. It fixes a regression where clang 4.0 can't build the Xen kernel due to some dependencies on DSE. The revert/reapply was for a supposedly flaky test case. The test case was problematic before, so that was nothing new.
Comment 5 Tom Stellard 2017-05-19 08:18:00 PDT 
Is there some less invasive way to fix the bug?
Comment 6 Jörg Sonnenberger 2017-05-19 08:27:40 PDT 
(In reply to Tom Stellard from comment #5)
> Is there some less invasive way to fix the bug?

Difficult, since the test infrastructure wants to really have two names for the pass. I'm not sure having an untested branch is an improvement.
Comment 7 Renato Golin 2017-05-20 13:18:18 PDT 
Adding folks in the original revision to get more eyes.

I still think this is too big (not just in size, but possibilities of things to go slightly wrong).

With llvm 5.0 coming out in July, I personally don't think we should merge in 4.0.1, but I'll leave it up to the people in the original review to have the final say.

cheers,
--renato
Comment 8 Eli Friedman 2017-05-22 13:07:39 PDT 
This seems too risky to me.  Changes to the optimizations we run early in the pass pipeline can have wide-reaching effects, and we want to minimize the risk of exposing new bugs in a dot release.
Comment 9 Tom Stellard 2017-05-24 03:11:27 PDT 
Based on feedback, I don't think we are going to be able to merge this patch.