Make ternary "__cond" flag 'const' for consistency of the final AST #12176
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
|
Thanks for your pull request and interest in making D better, @BorisCarvajal! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12176" |
kinke
added a commit
to kinke/ldc
that referenced
this pull request
Mar 6, 2021
This fixes dmd-testsuite's compilable/b16598.d by making sure to (partially) constant-fold dtor expressions of VarDeclarations too. dlang/dmd#12176 made these __cond temporaries const, triggering constant-folding and potentially getting rid of it altogether when optimizing a CondExp.
kinke
added a commit
to kinke/ldc
that referenced
this pull request
Mar 6, 2021
This fixes dmd-testsuite's compilable/b16598.d by making sure to (partially) constant-fold dtor expressions of VarDeclarations too. dlang/dmd#12176 made these __cond temporaries const, triggering constant-folding and potentially getting rid of it altogether when optimizing a CondExp.
kinke
added a commit
to kinke/dmd
that referenced
this pull request
Mar 7, 2021
PR dlang#12176 significantly changed the codegen AST for the compilable/b16598.d testcase. It's probably best to explain it with code: ----- struct S { this(int) {} ~this() {} } void main() { true ? S(1) : S(2); /* v2.095: -vcg-ast: (true) ? (S()).this(1) : (S()).this(2); actually: (bool __cond5 = true, __cond5) ? (S __slS3 = S(), __slS3).this(1) : (S __slS4 = S(), __slS4).this(2); dtor expressions for the 2 S temporaries: __slS3 edtor: __cond5 && __slS3.~this() __slS4 edtor: __cond5 || __slS4.~this() */ /* v2.096 with optimized CondExp due to __cond5 newly being const: -vcg-ast: (S()).this(1); actually: (S __slS3 = S(), __slS3).this(1); __slS3 edtor: __cond5 && __slS3.~this() */ } ----- LDC doesn't accept a reference to an undeclared __cond5 variable when codegen'ing the edtor; not sure why DMD doesn't complain. I've first looked into fixing this by being less aggressive when optimizing a CondExp: (const bool __cond5 = true, __cond5) ? <e1> : <e2> => currently: <e1> => 1st version: (const bool __cond5 = true, __cond5, e1) But I think it's better to optimize the dtor expressions of VarDeclarations and get rid of any constant __cond in there too. So with this patch, which also enables *partial* optimizations of LogicalExpressions (i.e., exploiting a const lhs for a non-const rhs too), the new codegen AST is: actually: (S __slS3 = S(), __slS3).this(1); __slS3 edtor: __slS3.~this()
dlang-bot
pushed a commit
that referenced
this pull request
Mar 7, 2021
PR #12176 significantly changed the codegen AST for the compilable/b16598.d testcase. It's probably best to explain it with code: ----- struct S { this(int) {} ~this() {} } void main() { true ? S(1) : S(2); /* v2.095: -vcg-ast: (true) ? (S()).this(1) : (S()).this(2); actually: (bool __cond5 = true, __cond5) ? (S __slS3 = S(), __slS3).this(1) : (S __slS4 = S(), __slS4).this(2); dtor expressions for the 2 S temporaries: __slS3 edtor: __cond5 && __slS3.~this() __slS4 edtor: __cond5 || __slS4.~this() */ /* v2.096 with optimized CondExp due to __cond5 newly being const: -vcg-ast: (S()).this(1); actually: (S __slS3 = S(), __slS3).this(1); __slS3 edtor: __cond5 && __slS3.~this() */ } ----- LDC doesn't accept a reference to an undeclared __cond5 variable when codegen'ing the edtor; not sure why DMD doesn't complain. I've first looked into fixing this by being less aggressive when optimizing a CondExp: (const bool __cond5 = true, __cond5) ? <e1> : <e2> => currently: <e1> => 1st version: (const bool __cond5 = true, __cond5, e1) But I think it's better to optimize the dtor expressions of VarDeclarations and get rid of any constant __cond in there too. So with this patch, which also enables *partial* optimizations of LogicalExpressions (i.e., exploiting a const lhs for a non-const rhs too), the new codegen AST is: actually: (S __slS3 = S(), __slS3).this(1); __slS3 edtor: __slS3.~this()
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.
This helps the optimizer to reduce both of the following cases to the same result (dead branch removed).
Of course backend is able to remove the dead branch but it doesn't get rid of the volatile var (it inserts an useless extra mov instruction), this looks like a defect/bug.
Also there is currently an assertion when generating debug info because of the previous statement, see #10630.