This patch folds icmp (select c,const,arg), null if arg has nonnullattr.
aqjune on Feb 14 2021, 5:17 AM.Authored by
It seems SimplifyCFG is introducing llvm.assume to preserve information after removing a conditional branch.
It sounds like a good idea, I updated the patch to insert assume(cond).
I think this is allowed only when nonnull is accompanied with noundef attribute:
I don't think this should be creating assumptions. It's pretty common for InstCombine transforms to lose some information while making folds, and we currently don't have any other InstCombine folds that try to preserve information through assumptions. Unless there is something specific to this transform that makes this particularly worthwhile, we should not deviate from the general pattern.
I think we can say that this is conceptually equivalent to SimplifyCFG that removes a conditional branch, and SimplifyCFG inserts an assumption when removing a condition (test9_null_callsite at llvm/test/Transforms/SimplifyCFG/UnreachableEliminate.ll), so this transformation is also chosen to insert assume.
The code duplication suggests that we should have a common place for this logic (especially if it already exists in SimplifyCFG).
Hmm, as you said, adding the "assume((select cond, null, p) != null)" pattern to isKnownNonZeroFromAssume seems enough?
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:
ATM I don't have a clear idea about how to implement it in ValueTracking without relying on assume..
BTW, before diving into ValueTracking's update, I made D97244 to make SimplifyCFG slightly stronger. As @ojeda mentioned in the bug report, dereferenceable implies noundef, and the attribute is pretty common, so I think it is a good idea to update this first and see how it goes.
Yeah, it is, although currently it is based on LLVM 11. For LLVM 12 support, take a look at https://github.com/rust-lang/rust/pull/81451. For LLVM 13, you probably need further tweaks.
When you configure Rust for building, use options like:
[llvm] download-ci-llvm = false skip-rebuild = true [target.x86_64-unknown-linux-gnu] llvm-config = "../your-custom-llvm-build/bin/llvm-config"
In general, take a look at config.toml.example, with src/bootstrap/defaults/config.codegen.toml as a start.
Instead of adding two transformations, I tweaked InstCombinerImpl::foldICmpInstWithConstantNotInt so it fires if icmp is comparing to null and select's true/false operands are either a constant or nonnullattr argument.
%.0.i = select i1 %2, i8* null, i8* %x %4 = icmp ne i8* %.0.i, null call void @llvm.assume(i1 %4) ret i8* %.0.1 => %.0.i = select i1 %2, i8* null, i8* %x %temp = xor i1 %2, 1 call void @llvm.assume(i1 %temp) ret i8* %.0.1 => ... ret i8* %x
BTW, fixing SimplifyCFG to replace phi operand with null doesn't work for the Rust example because phi is already folded into select before inlining. It roughly looks like this: https://godbolt.org/z/c38qon
I think it is hard to tell which version is more general - the previous patch was explicitly relying on uses that have nonnull attributes whereas the new one relies on assume calls.