[PATCH] D140347: SelectionDAG: Teach ComputeKnownBits about VSCALE

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3040-3051
+    unsigned Width = Multiplier.getActiveBits() + Log2_32(*MaxVScale) + 1;
+    APInt VScaleResultUpperbound =
+        APInt(Width, *MaxVScale) * Multiplier.sextOrTrunc(Width);
+
+    bool Negative = VScaleResultUpperbound.isNegative();
+    if (Negative)
+      VScaleResultUpperbound = ~VScaleResultUpperbound;
----------------
compnerd wrote:
> benmxwl-arm wrote:
> > foad wrote:
> > > craig.topper wrote:
> > > > compnerd wrote:
> > > > > foad wrote:
> > > > > > compnerd wrote:
> > > > > > > foad wrote:
> > > > > > > > compnerd wrote:
> > > > > > > > > foad wrote:
> > > > > > > > > > This seems pretty complex. Is the Multiplier operand guaranteed to have the same width as the result, i.e. BitWidth? If so you should be able to do all calculation in that width and the signedness of the multiplier should be irrelevant.
> > > > > > > > > No AFAIK, the multiplier has no guarantees of the width.  It may be 32-bit, or 64-bit, though LLVM did truncate it further.
> > > > > > > > Then the documentation for ISD::VSCALE should be clear about whether the multiplier is treated as signed or unsigned. I don't see it mentioned in ISDOpcodes.h.
> > > > > > > > 
> > > > > > > > Anyway, if it is signed, can't you first sextOrTrunc it to BitWdith, and then do all of the rest of the calculations in that width?
> > > > > > > My interpretation is that it is signed.  The LangRef calls it out as being a positive number, thus, it must be signed.  As to truncating it to BitWidth, again, that doesn't work.  We need it to be the computed width as we need to ensure that we have sufficient space for the multiplication to not overflow (which was the original bug) and we need it to match to perform the operation (requirements from APInt).  Is there a way to write this in a simpler way that I don't know about?
> > > > > > > The LangRef calls it out as being a positive number
> > > > > > No, that's talking about vscale itself. The multiplier is not mentioned in LangRef because it is specific to the ISD::VSCALE sdag node.
> > > > > > 
> > > > > > Anyway does this work:
> > > > > > ```
> > > > > >   Known.Zero.setBitsFrom(Log2_32(*MaxVScale) + 1);
> > > > > >   Known = KnownBits::mul(Known, KnownBits::makeConstant(Multiplier.sextOrTrunc(BitWidth)));
> > > > > > ```
> > > > > > ?
> > > > > ```
> > > > > void llvm::APInt::setBits(unsigned int, unsigned int): Assertion `loBit <= BitWidth && "loBit out of range"' failed.
> > > > > ```
> > > > > 
> > > > > This is why I had kept the "complex" code as is.  I had tried a few different things, I'd really rather prefer the `smul_ov` rather than the current implementation, but it seemed more complicated than this long-winded way.
> > > > The issue here is that we don't that vscale is *MaxVscale. We're trying to calculate an upper bound on the result of the multiply to determine the value of sign bits only.
> > > Fixing the assertion is easy:
> > > ```
> > > Known.Zero.setBitsFrom(std::min(Log2_32(*MaxVScale) + 1, BitWidth));
> > > Known = KnownBits::mul(Known, KnownBits::makeConstant(Multiplier.sextOrTrunc(BitWidth)));
> > > ```
> > `KnownBits::mul()` does not perform the same operation (and will under-report the know bits) and does not work for negative multipliers. 
> > 
> > (I did try using `KnownBits::mul()` in an earlier version of this patch)
> > 
> > Fixing the assertion is easy:
> 
> Truncating the value to `BitWidth` seems incorrect.  But also, as @benmxwl-arm mentions, that isn't exactly the same result.
> `KnownBits::mul()` does not perform the same operation (and will under-report the know bits)

Ack. I understand now that you want a result based on the actual maximum value of vscale, not just the known bits of vscale. Maybe value range propagation would be a better tool for this, but I guess we don't have that in SelectionDAG.


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

https://reviews.llvm.org/D140347