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

Simplify sadd_overflow+assume to add nsw [InstCombine] #80637

Closed
scottmcm opened this issue Feb 5, 2024 · 4 comments
Closed

Simplify sadd_overflow+assume to add nsw [InstCombine] #80637

scottmcm opened this issue Feb 5, 2024 · 4 comments
Labels
llvm:instcombine missed-optimization question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!

Comments

@scottmcm
Copy link

scottmcm commented Feb 5, 2024

The probably never comes up in C++, but in Rust people can easily end up there from things that boil down to

x.checked_add(y).unwrap_unchecked()

Proof: https://alive2.llvm.org/ce/z/m-n_PG

define i32 @src(i32 noundef %x, i32 noundef %y) noundef {
start:
  %#0 = sadd_overflow i32 noundef %x, noundef %y
  %_7.1 = extractvalue {i32, i1, i24} %#0, 1
  %_10 = xor i1 %_7.1, 1
  %_7.0 = extractvalue {i32, i1, i24} %#0, 0
  assume i1 %_10
  ret i32 %_7.0
}
=>
define i32 @tgt(i32 noundef %x, i32 noundef %y) noundef {
start:
  %r = add nsw i32 noundef %x, noundef %y
  ret i32 %r
}
Transformation seems to be correct!

And of course the uadd_overflow+assumeadd nuw too.

@nikic
Copy link
Contributor

nikic commented Feb 5, 2024

Do you have an example with larger context here?

As written, this is going to lose information, so I'm not sure whether this makes sense. I assume that there is some follow-on optimization failure that you want to avoid?

@scottmcm
Copy link
Author

scottmcm commented Feb 5, 2024

I don't have a specific case, since I've always been able to use .unchecked_add instead of the checked+unwrap_unchecked dance.

That said, I don't think it loses information? Alive says the opposite transform would also be legal: https://alive2.llvm.org/ce/z/cLbTUj

So AFAICT the nsw/nuw preserves all the necessary information.

@nikic
Copy link
Contributor

nikic commented Feb 5, 2024

The opposite transform is only legal because you are passing the value to a noundef return value. Otherwise this downgrades from UB to poison.

@scottmcm
Copy link
Author

scottmcm commented Feb 5, 2024

Ah, makes sense! Thanks. I guess I'll close this, then, since I didn't feel particularly strongly about it and you found a reason not to do it.

@scottmcm scottmcm closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2024
@dtcxzyw dtcxzyw removed their assignment Feb 5, 2024
@EugeneZelenko EugeneZelenko added the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label Feb 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 23, 2024
…=<try>

Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 23, 2024
…=<try>

Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 24, 2024
…=Amanieu

Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 25, 2024
…=Amanieu

Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
tiif pushed a commit to tiif/miri that referenced this issue Jun 25, 2024
Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang/rust#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 28, 2024
Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang/rust#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Jul 11, 2024
Also get `add nuw` from `uN::checked_add`

When I was doing this for `checked_{sub,shl,shr}`, it was mentioned rust-lang/rust#124114 (comment) that it'd be worth trying for `checked_add` too.

It makes a particularly-big difference for `x.checked_add(C)`, as doing this means that LLVM removes the intrinsic and does it as a normal `x <= MAX - C` instead.

cc `@DianQK` who had commented about `checked_add` related to rust-lang/hashbrown#509 before

cc llvm/llvm-project#80637 for how LLVM is unlikely to do this itself
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:instcombine missed-optimization question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!
Projects
None yet
Development

No branches or pull requests

4 participants