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

Move comparison before umul.with.overflow [instcombine?] #73306

Open
scottmcm opened this issue Nov 24, 2023 · 0 comments
Open

Move comparison before umul.with.overflow [instcombine?] #73306

scottmcm opened this issue Nov 24, 2023 · 0 comments

Comments

@scottmcm
Copy link

A bunch of containers have code enforcing object size limits that ends up looking like

let Some(total_bytes) = element_size.checked_mul(element_count) else { panic!() };
if total_bytes > (isize::MAX as usize) { panic!() };

in Rust, or in C++ something like

unsigned long total_bytes;
if __builtin_uaddl_overflow(element_size, element_count, &total_bytes) { throw bad_alloc(); }
if total_bytes > (size_t)PTR_DIFF_MAX { throw bad_alloc(); }

It would be nice if LLVM could -- when element_size is a constant, as it often ends up being in templated code -- move the check before the multiplication so that the overflow handling could be removed, and it just be a normal multiplication instead. (Perhaps even an nuw one, by sinking it into the branch that cares about the result.)

An example in Alive2: https://alive2.llvm.org/ce/z/X9ipzy

define {i64, i64} @src(i64 noundef %x) {
start:
  %#0 = umul_overflow i64 noundef %x, 26
  %_11.0.i = extractvalue {i64, i1, i24} %#0, 0
  %_11.1.i = extractvalue {i64, i1, i24} %#0, 1
  %_7.i.i = icmp ugt i64 %_11.0.i, 9223372036854775806
  %_0.sroa.0.0.i.i = select i1 %_7.i.i, i64 0, i64 2
  %#1 = insertvalue {i64, i64} poison, i64 %_0.sroa.0.0.i.i, 0
  %#2 = insertvalue {i64, i64} %#1, i64 %_11.0.i, 1
  %.pn.i = select i1 %_11.1.i, {i64, i64} { 0, -1 }, {i64, i64} %#2
  %.fca.0.extract = extractvalue {i64, i64} %.pn.i, 0
  %#3 = icmp eq i64 %.fca.0.extract, 0
  br i1 %#3, label %bb2, label %bb4

bb4:
  ret {i64, i64} %.pn.i

bb2:
  call void #trap() noreturn nothrow memory(inaccessiblemem: write)
  assume i1 0
}
=>
define {i64, i64} @tgt(i64 noundef %x) {
start:
  %_11.0.i = mul i64 noundef %x, 26
  %_7.i.i = icmp ugt i64 noundef %x, 354745078340568300
  %#1 = insertvalue {i64, i64} poison, i64 2, 0
  %#2 = insertvalue {i64, i64} %#1, i64 %_11.0.i, 1
  br i1 %_7.i.i, label %bb2, label %bb4

bb4:
  ret {i64, i64} %#2

bb2:
  call void #trap() noreturn nothrow memory(inaccessiblemem: write)
  assume i1 0
}
Transformation seems to be correct!

(It's based on https://rust.godbolt.org/z/47WWG4b4T, which was inspired by rust-lang/rust#118228.)

This issue is basically the same as #56563 but for umul.with.overflow rather than for ordinary mul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants