Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge r298799 into the 4.0 branch : Split the SimplifyCFG pass into two variants. #32109

Closed
jsonn opened this issue Apr 23, 2017 · 9 comments
Closed
Labels
bugzilla Issues migrated from bugzilla wontfix Issue is real, but we can't or won't fix it. Not invalid

Comments

@jsonn
Copy link
Contributor

jsonn commented Apr 23, 2017

Bugzilla Link 32762
Resolution WONTFIX
Resolved on May 24, 2017 03:11
Version 4.0
OS All
Blocks #31409
CC @deadalnix,@efriedma-quic,@jmolloy,@joker-eph,@rengolin,@tstellar

Extended Description

Is this patch OK to merge to the 4.0 branch?

@jsonn
Copy link
Contributor Author

jsonn commented Apr 23, 2017

Additionally, r298887 and r299558 for a problematic ARM/AArch64 test case.

@tstellar
Copy link
Collaborator

This looks like it will break the ABI. Also, this is a pretty big patch, does it fix a bug?

@rengolin
Copy link
Member

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...

@jsonn
Copy link
Contributor Author

jsonn commented Apr 26, 2017

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.

@tstellar
Copy link
Collaborator

Is there some less invasive way to fix the bug?

@jsonn
Copy link
Contributor Author

jsonn commented May 19, 2017

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.

@rengolin
Copy link
Member

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

@efriedma-quic
Copy link
Collaborator

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.

@tstellar
Copy link
Collaborator

Based on feedback, I don't think we are going to be able to merge this patch.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@Quuxplusone Quuxplusone added the wontfix Issue is real, but we can't or won't fix it. Not invalid label Jan 20, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla wontfix Issue is real, but we can't or won't fix it. Not invalid
Projects
None yet
Development

No branches or pull requests

5 participants