[PATCH] D79294: [InstSimplify] Remove known bits constant folding

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: test/Transforms/GVN/PRE/volatile.ll:204
 ; CHECK-NEXT:    [[LOAD:%.*]] = load volatile i32, i32* [[V:%.*]], !range !0
-; CHECK-NEXT:    ret i32 0
+; CHECK-NEXT:    ret i32 [[LOAD]]
 ;
----------------
spatel wrote:
> There's no dedicated fold for this in InstCombiner::visitLoadInst(). But we call computeKnownBits on the ret arg in instcombine anyway, so we get all basic patterns. That might be worth looking at as another candidate for removal for efficiency.
> 
> Ie, we may want to add a dedicated fold (add a simplifyLoad?) since it's cheap to do directly and a big win if it works (no idea if this happens in the real-world, but somebody added this GVN test case...)
> 
> A test that would subvert the current known-bits call from InstCombiner::visitReturnInst() based on current MaxDepth recursion:
> ```
> define i32 @known0(i32* %V, i32 %y) {
>   %load = load i32, i32* %V, !range !0
>   %m1 = mul i32 %load, %y
>   %m2 = mul i32 %m1, %y
>   %m3 = mul i32 %m2, %y
>   %m4 = mul i32 %m3, %y
>   %m5 = mul i32 %m4, %y
>   %m6 = mul i32 %m5, %y
>   ret i32 %m6
> }
> 
> !0 = !{ i32 0, i32 1 }
> 
> ```
Going through the blame, this test was added in https://github.com/llvm/llvm-project/commit/b8da3a2bb2b840db6ab7c473190ee6d65dcf3a1e. I think the intention was to make sure that instructions with side-effects don't get optimized away just because they simplify. Given that, it might make sense to replace the load volatile with something else here (not sure what though -- do we have any standard pattern that simplifies without being trivially dead?)

I don't think we need to explicitly handle this case in InstCombine, as it seems unlikely that single-element range annotations are common in the wild. Additionally I think that this is best left to passes that specialize in range-propagation. For example, CVP will also handle your more complex example successfully. Given ongoing range work, I expect that SCCP will also handle it in the future.


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

https://reviews.llvm.org/D79294