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.

Sign up for GitHub

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

Jump to bottom

Regressions from bc886e9b587b9d009f49b12eaaa9ebc1c71905a1 #58717

Closed
glandium opened this issue Nov 1, 2022 · 3 comments
Closed

Regressions from bc886e9b587b9d009f49b12eaaa9ebc1c71905a1 #58717

glandium opened this issue Nov 1, 2022 · 3 comments
Labels
llvm:instcombine

Comments

@glandium
Copy link
Contributor

glandium commented Nov 1, 2022

  • In Firefox, this causes multiple errors in e.g. the PNG test suite (which look like off-by-one in some computations).
  • In clang itself (when built with a clang that contains bc886e9), this causes "error: out of range pc-relative fixup value" when compiling Firefox for ARMv7 android. (reproducer: https://gist.github.com/glandium/273f01e7c825750854e68d9bf6049d4f ; I didn't try to reduce the source)

Both the failures above have been bisected to bc886e9.

Cc: @rotateright
@jrtc27
Copy link
Collaborator

jrtc27 commented Nov 1, 2022

bc886e9#diff-efa3000dc7eaa975d0cbb3243b500a049024a2c5698d4d6666f72e87769a9c9dR182 should be CreateSub

@glandium
Copy link
Contributor Author

glandium commented Nov 1, 2022

This was apparently reverted in e1de7ac. https://reviews.llvm.org/rGbc886e9b587b9d009f49b12eaaa9ebc1c71905a1

@glandium glandium closed this as completed Nov 1, 2022
@rotateright
Copy link
Contributor

Thanks for the report and analysis! Sorry about the bug.

Yes, the "add" that was copied from the earlier code was supposed to be a "sub", and I missed it.

On the plus side, I wasn't sure if this pattern existed in real code, but the fallout from screwing up the patch says it occurs quite a bit. :)

rotateright referenced this issue Nov 1, 2022
@rotateright
[InstCombine] fold mul with decremented "shl -1" factor
bc886e9 
This is a sibling to:
6064e92
...but we canonicalize the shl+add to shl+xor,
so the pattern is different than I expected:
https://alive2.llvm.org/ce/z/8CX16e

I have not found any patterns that are safe
to propagate no-wrap, so that is not included
here.
@rotateright rotateright mentioned this issue Nov 1, 2022
Closed
rotateright added a commit that referenced this issue Nov 2, 2022
@rotateright
[InstCombine] fold mul with decremented "shl -1" factor (2nd try)
f03b069 
This is a corrected version of:
bc886e9

I made a copy-paste error that created an "add" instead of the
intended "sub" on that attempt. The regression tests showed the
bug, but I overlooked that.

As I said in a comment on issue #58717, the bug reports resulting
from the botched patch confirm that the pattern does occur in
many real-world applications, so hopefully eliminating the multiply
results in better code.

I added one more regression test in this version of the patch,
and here's an Alive2 proof to show that exact example:
https://alive2.llvm.org/ce/z/dge7VC

Original commit message:

This is a sibling to:
6064e92
...but we canonicalize the shl+add to shl+xor,
so the pattern is different than I expected:
https://alive2.llvm.org/ce/z/8CX16e

I have not found any patterns that are safe
to propagate no-wrap, so that is not included
here.

Differential Revision: https://reviews.llvm.org/D137157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
llvm:instcombine
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jrtc27 @glandium @EugeneZelenko @rotateright