[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]]
 ;
----------------
nikic wrote:
> 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.
> 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?)

Looks like `call i32 undef()` works for that purpose. I've replaced the load volatile with that.


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

https://reviews.llvm.org/D79294