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

Missed simplifications when shl is used instead of mul #46928

Closed
davidbolvansky opened this issue Sep 19, 2020 · 5 comments
Closed

Missed simplifications when shl is used instead of mul #46928

davidbolvansky opened this issue Sep 19, 2020 · 5 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@davidbolvansky
Copy link
Collaborator

Bugzilla Link 47584
Resolution FIXED
Resolved on Sep 20, 2020 10:34
Version trunk
OS Linux
CC @fhahn,@RKSimon,@rotateright
Fixed by commit(s) 7903ae4

Extended Description

int foo(int p)
{
unsigned i, s = 0;
for (i = 4; i < 20; i++)
{
if (p)
s += i2; // works for i3
else
s += i*4;
}
return s;
}

Clang produces:
foo(int): # @​foo(int)
cmp edi, 1
mov cl, 1
adc cl, 0
mov eax, 4
shl eax, cl
mov edx, 5
shl edx, cl
mov esi, 6
shl esi, cl
add edx, eax
add esi, edx
mov eax, 7
shl eax, cl
add eax, esi
mov edx, 8
shl edx, cl
mov esi, 9
shl esi, cl
mov edi, 10
shl edi, cl
add edx, eax
add esi, edx
....

But it should just produce:
foo(int): # @​foo(int)
test edi, edi
mov ecx, 736
mov eax, 368
cmove eax, ecx
ret

@fhahn
Copy link
Contributor

fhahn commented Sep 19, 2020

Look like this is a case where we miss hoisting the common operand from the multiplications when shl is used instead of mul: https://godbolt.org/z/66cqMK

@RKSimon
Copy link
Collaborator

RKSimon commented Sep 19, 2020

rewriting as:

int foo2(int p)
{
unsigned i, s = 0;
if (p) {
for (i = 4; i < 20; i++)
s += i2; // works for i3
} else {
for (i = 4; i < 20; i++)
s += i4; // works for i3
}
return s;
}

gives:

_Z4foo2i:
testl %edi, %edi
movl $736, %ecx # imm = 0x2E0
movl $368, %eax # imm = 0x170
cmovel %ecx, %eax
retq

@davidbolvansky
Copy link
Collaborator Author

Interesting, so failed loop unswitch for original test case?

@rotateright
Copy link
Contributor

Look like this is a case where we miss hoisting the common operand from the
multiplications when shl is used instead of mul:
https://godbolt.org/z/66cqMK

Instcombine is definitely missing a factorization for shl with add/sub. I was looking at that fold as part of solving bug 47430.

I added some tests here:
1dd4c4e

Let me see if adding that fixes the example in the description.

@rotateright
Copy link
Contributor

With the instcombine improvement, the original example collapses to the optimal code:
7903ae4

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 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

4 participants