[PATCH] D154953: [InstSimplify] Remove the remainder loop if we know the mask is always true

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1286
+      unsigned MaxActiveBits = Known.countMaxActiveBits();
+      if (MaxActiveBits <= RemC->logBase2())
+        return ConstantInt::getNullValue(Op0->getType());
----------------
Allen wrote:
> nikic wrote:
> > Allen wrote:
> > > nikic wrote:
> > > > This check looks a bit roundabout. I think what you actually want to check is that `Known.getMaxValue().ule(*RemC)`?
> > > I still use the **getActiveBits**  because the value of **Known.getMaxValue()** is not a power-of-two value. 
> > > For example, we can easily get the the ConstantRange(8, 16) from the **vscale_range(1,16)**, but then the 
> > > **Known = ConstantRange(8, 16)->toKnownBits()** will get the value of **Known.getMaxValue()** is 31 instead of 16,
> > > so I can't compare them with its value directly.
> > > 
> > > I also change the boundary test for test case **urem_vscale_range **and **urem_shl_vscale_out_of_range**
> > Okay, I see. Would it work if you used computeConstantRange() instead of computeKnownBits()? That should give an accurate range for vscale.
> thanks, the new API **computeConstantRange  **works
Hrm, it looks like computeConstantRange() only works with an additional special case for shl of vscale. That's ... not great, and probably fragile, because the same thing will happen with more complex patterns.

I think now that I understand why you did this, I would prefer to go back to your previous patch that used getActiveBits(). Just add a comment that we need to use getActiveBits() to make use of the additional power of two knowledge.


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

https://reviews.llvm.org/D154953