[PATCH] D35040: [IR] Implement Constant::isNegativeZeroValue/isZeroValue/isAllOnesValue/isOneValue/isMinSignedValue for ConstantDataVector without going through getElementAsConstant
Craig Topper via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 10 12:32:07 PDT 2017
~Craig
On Mon, Jul 10, 2017 at 12:08 PM, Justin Bogner <mail at justinbogner.com>
wrote:
> Craig Topper via Phabricator <reviews at reviews.llvm.org> writes:
> > craig.topper created this revision.
> >
> > Currently these methods call ConstantDataVector::getSplatValue which
> > uses getElementsAsConstant to create a Constant object representing
> > the element value. This method incurs a map lookup to see if we
> > already have created such a Constant before and if not allocates a new
> > Constant object.
> >
> > This patch changes these methods to use getElementAsAPFloat and
> > getElementAsInteger so we can just examine the data values directly.
>
> LGTM with a couple of comments.
>
> > https://reviews.llvm.org/D35040
> >
> > Files:
> > include/llvm/IR/Constants.h
> > lib/IR/Constants.cpp
> >
> > Index: lib/IR/Constants.cpp
> > ===================================================================
> > --- lib/IR/Constants.cpp
> > +++ lib/IR/Constants.cpp
> > @@ -44,9 +44,10 @@
> >
> > // Equivalent for a vector of -0.0's.
> > if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(
> CV->getSplatValue()))
> > - if (SplatCFP && SplatCFP->isZero() && SplatCFP->isNegative())
> > - return true;
> > + if (CV->isSplat())
> > + if (CV->getElementType()->isFloatingPointTy())
> > + if (CV->getElementAsAPFloat(0).isNegZero())
> > + return true;
> >
> > if (const ConstantVector *CV = dyn_cast<ConstantVector>(this))
> > if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(
> CV->getSplatValue()))
> > @@ -70,9 +71,10 @@
> >
> > // Equivalent for a vector of -0.0's.
> > if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(
> CV->getSplatValue()))
> > - if (SplatCFP && SplatCFP->isZero())
> > - return true;
> > + if (CV->isSplat())
> > + if (CV->getElementType()->isFloatingPointTy())
> > + if (CV->getElementAsAPFloat(0).isZero())
> > + return true;
> >
> > if (const ConstantVector *CV = dyn_cast<ConstantVector>(this))
> > if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(
> CV->getSplatValue()))
> > @@ -113,9 +115,14 @@
> > return Splat->isAllOnesValue();
> >
> > // Check for constant vectors which are splats of -1 values.
> > - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (Constant *Splat = CV->getSplatValue())
> > - return Splat->isAllOnesValue();
> > + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(this))
> {
> > + if (CV->isSplat()) {
> > + Type *Ty = CV->getElementType();
> > + if (Ty->isFloatingPointTy())
>
> I think these cases might technically change the behaviour (though if
> I'm right the current behaviour is probably a bug). AFAICT
> getElementAsConstant hand rolls its own isFloatingPointTy that doesn't
> handle the more exotic floating point types:
>
> if (getElementType()->isHalfTy() || getElementType()->isFloatTy() ||
> getElementType()->isDoubleTy())
> return ConstantFP::get(getContext(), getElementAsAPFloat(Elt));
>
> So I guess we should probably fix getElementAsConstant to use
> isFloatingPointTy as well, as a follow up.
>
A ConstantDataVector never contains any exotic FP types as far as I know.
>
> > + return CV->getElementAsAPFloat(0).bitcastToAPInt().
> isAllOnesValue();
> > + return CV->getElementAsInteger(0) == maxUIntN(Ty->
> getIntegerBitWidth());
> > + }
> > + }
> >
> > return false;
> > }
> > @@ -135,9 +142,14 @@
> > return Splat->isOneValue();
> >
> > // Check for constant vectors which are splats of 1 values.
> > - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (Constant *Splat = CV->getSplatValue())
> > - return Splat->isOneValue();
> > + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(this))
> {
> > + if (CV->isSplat()) {
> > + Type *Ty = CV->getElementType();
> > + if (Ty->isFloatingPointTy())
> > + return CV->getElementAsAPFloat(0).
> bitcastToAPInt().isOneValue();
> > + return CV->getElementAsInteger(0) == 1;
> > + }
> > + }
> >
> > return false;
> > }
> > @@ -157,9 +169,15 @@
> > return Splat->isMinSignedValue();
> >
> > // Check for constant vectors which are splats of INT_MIN values.
> > - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (Constant *Splat = CV->getSplatValue())
> > - return Splat->isMinSignedValue();
> > + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(this))
> {
> > + if (CV->isSplat()) {
> > + Type *Ty = CV->getElementType();
> > + if (Ty->isFloatingPointTy())
> > + return CV->getElementAsAPFloat(0).bitcastToAPInt().
> isMinSignedValue();
> > + return CV->getElementAsInteger(0) ==
> > + -(uint64_t)minIntN(Ty->getIntegerBitWidth());
>
> Should we add a getElementAsAPInt instead to avoid having to manually
> work out isMinSignedValue() and friends?
>
Yeah that's probably better. This is now the second time I've tried to bend
minIntN to my will. The first being in APInt's implementation of
isMinSignedValue...
>
> > + }
> > + }
> >
> > return false;
> > }
> > @@ -179,9 +197,15 @@
> > return Splat->isNotMinSignedValue();
> >
> > // Check for constant vectors which are splats of INT_MIN values.
> > - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(
> this))
> > - if (Constant *Splat = CV->getSplatValue())
> > - return Splat->isNotMinSignedValue();
> > + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(this))
> {
> > + if (CV->isSplat()) {
> > + Type *Ty = CV->getElementType();
> > + if (Ty->isFloatingPointTy())
> > + return !CV->getElementAsAPFloat(0).bitcastToAPInt().
> isMinSignedValue();
> > + return CV->getElementAsInteger(0) !=
> > + -(uint64_t)minIntN(Ty->getIntegerBitWidth());
> > + }
> > + }
> >
> > // It *may* contain INT_MIN, we can't tell.
> > return false;
> > @@ -2627,17 +2651,21 @@
> > return Str.drop_back().find(0) == StringRef::npos;
> > }
> >
> > -Constant *ConstantDataVector::getSplatValue() const {
> > +bool ConstantDataVector::isSplat() const {
> > const char *Base = getRawDataValues().data();
> >
> > // Compare elements 1+ to the 0'th element.
> > unsigned EltSize = getElementByteSize();
> > for (unsigned i = 1, e = getNumElements(); i != e; ++i)
> > if (memcmp(Base, Base+i*EltSize, EltSize))
> > - return nullptr;
> > + return false;
> >
> > + return true;
> > +}
> > +
> > +Constant *ConstantDataVector::getSplatValue() const {
> > // If they're all the same, return the 0th one as a representative.
> > - return getElementAsConstant(0);
> > + return isSplat() ? getElementAsConstant(0) : nullptr;
> > }
> >
> > //===-------------------------------------------------------
> ---------------===//
> > Index: include/llvm/IR/Constants.h
> > ===================================================================
> > --- include/llvm/IR/Constants.h
> > +++ include/llvm/IR/Constants.h
> > @@ -761,6 +761,10 @@
> > /// i32/i64/float/double) and must be a ConstantFP or ConstantInt.
> > static Constant *getSplat(unsigned NumElts, Constant *Elt);
> >
> > + /// Returns true if this is a splat constant, meaning that all
> elements have
> > + /// the same value.
> > + bool isSplat() const;
> > +
> > /// If this is a splat constant, meaning that all of the elements
> have the
> > /// same value, return that value. Otherwise return NULL.
> > Constant *getSplatValue() const;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/f8485fa9/attachment.html>
More information about the llvm-commits
mailing list