Address issue 4369 "as best we can" #4441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After extensive investigation of the weirdness that is #4369, Joe and I believe the issue is most likely an LLVM bug. However, we don't have time to pursue further. During the investigation, I came to believe that the change to use CodeGenOpt::Default instead of CodeGenOpt::None for target optimizations would be prudent. It prevents the known instances of bugs like #4369 which is nice but not key to this change. Changes things because they make symptoms go away isn't a good strategy if you don't understand why. We do not "fully" understand the impact of this change on bugs like #4369. However, it appears that CodeGenOpt::None is a code path that sees little use or testing and is more prone to bugs/regression. Given that we have seen more than 1 bug that "smells like" #4369, we felt it was wise to switch to Default from None at least until such time as we understand #4369 and #3874. LLVM is a complicated beast and using code paths that are little used can be a dangerous proposition. This commit doesn't include release notes as we addresses to user facing instance of #4369 already.
ponylang-main
added
the
discuss during sync
ponylang-main
removed
the
discuss during sync
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After extensive investigation of the weirdness that is #4369, Joe and I believe the issue is most likely an LLVM bug. However, we don't have time to pursue further.
During the investigation, I came to believe that the change to use CodeGenOpt::Default instead of CodeGenOpt::None for target optimizations would be prudent.
It prevents the known instances of bugs like #4369 which is nice but not key to this change. Changes things because they make symptoms go away isn't a good strategy if you don't understand why. We do not "fully" understand the impact of this change on bugs like #4369. However, it appears that CodeGenOpt::None is a code path that sees little use or testing and is more prone to bugs/regression.
Given that we have seen more than 1 bug that "smells like" #4369, we felt it was wise to switch to Default from None at least until such time as we understand #4369 and #3874. LLVM is a complicated beast and using code paths that are little used can be a dangerous proposition.
This commit doesn't include release notes as we addresses to user facing instance of #4369 already.