Skip to content

Regression: Missed fold for x * 0 #50630

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

Closed
jeremy-rifkin mannequin opened this issue Jul 31, 2021 · 5 comments
Closed

Regression: Missed fold for x * 0 #50630

jeremy-rifkin mannequin opened this issue Jul 31, 2021 · 5 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@jeremy-rifkin
Copy link
Mannequin

jeremy-rifkin mannequin commented Jul 31, 2021

Bugzilla Link 51286
Resolution FIXED
Resolved on Sep 15, 2021 06:10
Version trunk
OS Windows NT
Blocks #49274
CC @nerh,@RKSimon,@rotateright
Fixed by commit(s) f5d8952

Extended Description

Clang trunk is not generating optimal code for a (very poor) multiplication routine https://godbolt.org/z/GrjMhsTcv.

int mul(int x, int y) {
    int p = 0;
    while(x--) p += y;
    return p;
}

Should compile to:

mul(int, int):
        mov     eax, edi
        imul    eax, esi
        ret

And it does in clang 12 but not in clang trunk.

If I'm not mistaken it appears a select i1 (icmp eq i32 %x, 0), i32 0, i32 (mul i32 %x, %y) -> mul i32 %x, %y fold is not firing.

@jeremy-rifkin
Copy link
Mannequin Author

jeremy-rifkin mannequin commented Jul 31, 2021

assigned to @nerh

@ghost
Copy link

ghost commented Aug 15, 2021

Aforementioned code snipped is no longer folded into single multiplication instruction after changes in "simplifyWithOpReplaced".

If there are no objections I'd like to take this issue fix the optimization.

@RKSimon
Copy link
Collaborator

RKSimon commented Aug 15, 2021

SGTM

Current IR:

define i32 @mul(i32 %0, i32 %1) {
  %3 = icmp eq i32 %0, 0
  %4 = mul i32 %1, %0
  %5 = select i1 %3, i32 0, i32 %4
  ret i32 %5
}

To get rid of that select we will need freeze: https://alive2.llvm.org/ce/z/maX0k-

@ghost
Copy link

ghost commented Aug 21, 2021

Candidate patch: https://reviews.llvm.org/D108408

@rotateright
Copy link
Contributor

We have the expected codegen again (just a multiply, no cmov) after:
https://reviews.llvm.org/rGf5d89523567b

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
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
Projects
None yet
Development

No branches or pull requests

2 participants