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

Bad codegen for comparison with postfix #4651

Open
rikkimax opened this issue May 8, 2024 · 5 comments · May be fixed by #4656
Open

Bad codegen for comparison with postfix #4651

rikkimax opened this issue May 8, 2024 · 5 comments · May be fixed by #4656

Comments

@rikkimax
Copy link
Contributor

rikkimax commented May 8, 2024

ldc fe 2.107 as per run.dlang.org

void main()
{
    int d = 42;
    bool o = d > d++;
}
define i32 @_Dmain({ i64, ptr } %unnamed) #0 {
  %d = alloca i32, align 4                        ; [#uses = 4, size/byte = 4]
  %o = alloca i8, align 1                         ; [#uses = 1, size/byte = 1]
  store i32 42, ptr %d, align 4
  %1 = load i32, ptr %d, align 4                  ; [#uses = 2]
  %2 = add i32 %1, 1                              ; [#uses = 1]
  store i32 %2, ptr %d, align 4
  %3 = load i32, ptr %d, align 4                  ; [#uses = 1]
  %4 = icmp sgt i32 %3, %1                        ; [#uses = 1]
  %5 = zext i1 %4 to i8                           ; [#uses = 1]
  store i8 %5, ptr %o, align 1
  ret i32 0
}

Note; the icmp is after the add, which it shouldn't be.

Source: https://twitter.com/marcos_don/status/1787629240550150361

@kinke
Copy link
Member

kinke commented May 8, 2024

Thx for the report. The problem isn't the icmp after the add (that's fine), but we need to load the lhs eagerly, not using it as lvalue and loading it freshly before the icmp (so after the add + store).

@JohanEngelen
Copy link
Member

JohanEngelen commented May 17, 2024

Unclear what the exact bug is, see this post by @ibuclaw https://forum.dlang.org/post/[email protected]

Perhaps a > b is undefined/implementation-defined when evaluation of a or b changes the evaluation of the other.

@kinke
Copy link
Member

kinke commented May 17, 2024

Yeah as can be seen from the diff in #4656, this wouldn't be doable for all cases anyway, most notably if an operand is a struct or class, since that would require creating temporaries, exactly as Iain wrote in https://forum.dlang.org/post/[email protected]. So I put this on hold for now, as I wouldn't like diverging operand load orders depending on whether it's a primitive or aggregate type.

@JohanEngelen
Copy link
Member

@rikkimax Please submit an upstream bug report, to find out what the correct solution is here (e.g. improve the spec).

@rikkimax
Copy link
Contributor Author

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 a pull request may close this issue.

3 participants