[PATCH] D96663: [InstCombine] Fold nonnull (select c,null,p) to nonnull p

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D96663#2578084 <https://reviews.llvm.org/D96663#2578084>, @aqjune wrote:

> In D96663#2577600 <https://reviews.llvm.org/D96663#2577600>, @spatel wrote:
>
>> The code duplication suggests that we should have a common place for this logic (especially if it already exists in SimplifyCFG). 
>> Could it be added to ValueTracking's isKnownNonZero() or wrap around that? 
>> It might already be in there under isKnownNonNullFromDominatingCondition(), but we need to fix it up to deal with selects?
>
> Hmm, as you said, adding the "assume((select cond, null, p) != null)" pattern to isKnownNonZeroFromAssume seems enough?
> Compiling the Rust code snippet from pr48975 already creates llvm.assume: https://godbolt.org/z/8KxWKP
> That being said, it might be a good idea to update `passingValueIsAlwaysUndefined` to check `CB->paramHasAttr(ArgIdx, Attribute::Dereferenceable)` as well because `dereferenceable` is quite common. `dereferenceable` implies `noundef`, so it is valid to assume that passing null to `nonnull dereferenceable` is UB. What do you think?

If we can make the value tracking analysis better without adding or relying on llvm.assume, that would be best. I'm not sure about the current thinking on assume intrinsics, but we had found cases where the extra instructions/uses were interfering with optimization rather than helping. We've also seen cases like https://llvm.org/PR49171 where the cost of analyzing assumes is extreme. Not sure if those problems are overcome by assume bundles:
https://llvm.org/docs/LangRef.html#assume-operand-bundles


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96663