[PATCH] D140851: [InstCombine]: Add cases for assume (X & Y != {0|Y})

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added a comment.

In D140851#4040038 <https://reviews.llvm.org/D140851#4040038>, @nikic wrote:

> The implementation looks basically fine. I think it would be easiest to land this if it's detached from the patch stack, with some dedicated tests -- which would probably be pretty much just these?

Done, test patch is: https://reviews.llvm.org/D141412

>   declare void @llvm.assume(i1)
>   
>   define i32 @pow2(i32 %x) {
>     %and = and i32 %x, 4
>     %cmp = icmp ne i32 %and, 0
>     call void @llvm.assume(i1 %cmp)
>     %and2 = and i32 %x, 4
>     ret i32 %and2
>   }
>   
>   define i32 @not_pow2(i32 %x) {
>     %and = and i32 %x, 3
>     %cmp = icmp ne i32 %and, 0
>     call void @llvm.assume(i1 %cmp)
>     %and2 = and i32 %x, 3
>     ret i32 %and2
>   }

Added those tests along with some other variations.
Some of which will be "fixed" in D140852 <https://reviews.llvm.org/D140852> (the `_br` cases) and others
that need seperate patches (i.e if the power of 2 is the sign bit
we canonicalize to `icmp slt %x, 0` which causes missed optimization
(see `pow_2_8_assume`).

Also added some tests for when the power of 2 is non-constant but
we still "should" be able to prove we don't need the operation (will
look into that more later).

My plan is:

1. get this in
2. Split `D140852` from the series and submit that (it will cleanup some cases here)
3. Split `D140850` and `D140849` and independent patches (just the mul case)
4. New patch series for `slt` and non-constant cases.

That sound reasonable?

---

Also, if you have a moment about an unrelated thing. I'm working on a patch
for `SimplifyLibCalls` to see if there is a previous dominating `slen = strlen(s)` call
that will allow transformation for `str*(s, ...)` -> `mem*(s, ..., slen)` even if the string is non-constant.
To do this, it needs to check that between the `strlen` call and lib-call the memory
`*s` can't have been clobbered. I think the "right" way to do this is `memoryssa` but
it seems ridiculous (and overly consuming of compile time) to add `memoryssa` to all
of `instcombine` just for this one relatively unimportant pass. I saw we have `AAResults`
already computed so started with that, but it only works on a single basic-block (and since
we don't have `postdom` tree there doesn't seem to be any cheap way to collect all possible
affected basic-blocks between the `strlen` call and lib-call. My thought for how to handle
this was either:

1. Bite the bullet and use `memoryssa` throughout `instcombine`.
2. Only handle `strlen` calls in the same basic-block as the lib-call and use `AAResults`
3. Add a file to "canonicalize" lib calls before EarlyCSE by making ALL `str*(s,...)` functions `mem*(s, ..., strlen(s))`, let `EarlyCSE` remove all duplicate `strlen` calls, then make the `SimplifyLibCall` in `instcombine` replace `mem*(s, ..., strlen(s))` with the `str*` equivilent.

Do you have any advise about which of these (if any) make the most sense. And if none
how you might approach it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140851/new/

https://reviews.llvm.org/D140851