Skip to content

fix #21435 Bad codegen when building dmd with optimizations #21436

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

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

rainers
Copy link
Member

@rainers rainers commented Jun 8, 2025

don't handle postinc/dec of *p++ again in cdeq(), it is already done in cdpost() and only modifies the temporary copy, sometimes even a different register.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @rainers!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

  • In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#21436"

@Herringway
Copy link
Contributor

It doesn't look like the test is being built with optimizations?

don't handle postinc/dec of *p++ again in cdeq(), it is already done in cdpost() and only modifies the temporary copy, sometimes even a different register.
@rainers
Copy link
Member Author

rainers commented Jun 8, 2025

It doesn't look like the test is being built with optimizations?

Tests should be built with all combinations of -g -O -inline -release, all but -g seem to be necessary to trigger the bad register usage.

@@ -805,7 +805,7 @@ void cdeq(ref CGstate cg, ref CodeBuilder cdb,elem* e,ref regm_t pretregs)

fixresult(cdb,e,retregs,pretregs);
Lp:
if (postinc)
if (postinc && false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @WalterBright , is it actually ok to disable this code? It generates the gratuitious add after *p++ = 'A', e.g.:

0000000000000107: 48 8B D9           mov         rbx,rcx
000000000000010A: 48 FF C1           inc         rcx
000000000000010D: C6 03 41           mov         byte ptr [rbx],41h
0000000000000110: 48 83 C3 01        add         rbx,1

Is it used in some situation not covered by the test suite? If not, should the use of postinc be removed completely from cdeq?

I suspect the bug causing RSP to be modified by the addition is still lurking in idxregm, though: if the used register happens to be R12, this condition

if ((rm & 7) == 4) /* if sib byte */

is true and idxregm returns a mask with bits 4 and 12 set, and findreg then selects 4 (RSP).

@rainers rainers requested a review from WalterBright June 14, 2025 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants