<div dir="ltr"><br><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature">~Craig</div></div>
<br><div class="gmail_quote">On Mon, Jul 10, 2017 at 12:08 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Craig Topper via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
> craig.topper created this revision.<br>
><br>
> Currently these methods call ConstantDataVector::<wbr>getSplatValue which<br>
> uses getElementsAsConstant to create a Constant object representing<br>
> the element value. This method incurs a map lookup to see if we<br>
> already have created such a Constant before and if not allocates a new<br>
> Constant object.<br>
><br>
> This patch changes these methods to use getElementAsAPFloat and<br>
> getElementAsInteger so we can just examine the data values directly.<br>
<br>
LGTM with a couple of comments.<br>
<br>
> <a href="https://reviews.llvm.org/D35040" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D35040</a><br>
><br>
> Files:<br>
> include/llvm/IR/Constants.h<br>
> lib/IR/Constants.cpp<br>
><br>
> Index: lib/IR/Constants.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/IR/Constants.cpp<br>
> +++ lib/IR/Constants.cpp<br>
> @@ -44,9 +44,10 @@<br>
><br>
> // Equivalent for a vector of -0.0's.<br>
> if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(<wbr>CV->getSplatValue()))<br>
> - if (SplatCFP && SplatCFP->isZero() && SplatCFP->isNegative())<br>
> - return true;<br>
> + if (CV->isSplat())<br>
> + if (CV->getElementType()-><wbr>isFloatingPointTy())<br>
> + if (CV->getElementAsAPFloat(0).<wbr>isNegZero())<br>
> + return true;<br>
><br>
> if (const ConstantVector *CV = dyn_cast<ConstantVector>(this)<wbr>)<br>
> if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(<wbr>CV->getSplatValue()))<br>
> @@ -70,9 +71,10 @@<br>
><br>
> // Equivalent for a vector of -0.0's.<br>
> if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(<wbr>CV->getSplatValue()))<br>
> - if (SplatCFP && SplatCFP->isZero())<br>
> - return true;<br>
> + if (CV->isSplat())<br>
> + if (CV->getElementType()-><wbr>isFloatingPointTy())<br>
> + if (CV->getElementAsAPFloat(0).<wbr>isZero())<br>
> + return true;<br>
><br>
> if (const ConstantVector *CV = dyn_cast<ConstantVector>(this)<wbr>)<br>
> if (ConstantFP *SplatCFP = dyn_cast_or_null<ConstantFP>(<wbr>CV->getSplatValue()))<br>
> @@ -113,9 +115,14 @@<br>
> return Splat->isAllOnesValue();<br>
><br>
> // Check for constant vectors which are splats of -1 values.<br>
> - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (Constant *Splat = CV->getSplatValue())<br>
> - return Splat->isAllOnesValue();<br>
> + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this)) {<br>
> + if (CV->isSplat()) {<br>
> + Type *Ty = CV->getElementType();<br>
> + if (Ty->isFloatingPointTy())<br>
<br>
I think these cases might technically change the behaviour (though if<br>
I'm right the current behaviour is probably a bug). AFAICT<br>
getElementAsConstant hand rolls its own isFloatingPointTy that doesn't<br>
handle the more exotic floating point types:<br>
<br>
if (getElementType()->isHalfTy() || getElementType()->isFloatTy() ||<br>
getElementType()->isDoubleTy()<wbr>)<br>
return ConstantFP::get(getContext(), getElementAsAPFloat(Elt));<br>
<br>
So I guess we should probably fix getElementAsConstant to use<br>
isFloatingPointTy as well, as a follow up.<br></blockquote><div><br></div><div>A ConstantDataVector never contains any exotic FP types as far as I know. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + return CV->getElementAsAPFloat(0).<wbr>bitcastToAPInt().<wbr>isAllOnesValue();<br>
> + return CV->getElementAsInteger(0) == maxUIntN(Ty-><wbr>getIntegerBitWidth());<br>
> + }<br>
> + }<br>
><br>
> return false;<br>
> }<br>
> @@ -135,9 +142,14 @@<br>
> return Splat->isOneValue();<br>
><br>
> // Check for constant vectors which are splats of 1 values.<br>
> - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (Constant *Splat = CV->getSplatValue())<br>
> - return Splat->isOneValue();<br>
> + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this)) {<br>
> + if (CV->isSplat()) {<br>
> + Type *Ty = CV->getElementType();<br>
> + if (Ty->isFloatingPointTy())<br>
> + return CV->getElementAsAPFloat(0).<wbr>bitcastToAPInt().isOneValue();<br>
> + return CV->getElementAsInteger(0) == 1;<br>
> + }<br>
> + }<br>
><br>
> return false;<br>
> }<br>
> @@ -157,9 +169,15 @@<br>
> return Splat->isMinSignedValue();<br>
><br>
> // Check for constant vectors which are splats of INT_MIN values.<br>
> - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (Constant *Splat = CV->getSplatValue())<br>
> - return Splat->isMinSignedValue();<br>
> + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this)) {<br>
> + if (CV->isSplat()) {<br>
> + Type *Ty = CV->getElementType();<br>
> + if (Ty->isFloatingPointTy())<br>
> + return CV->getElementAsAPFloat(0).<wbr>bitcastToAPInt().<wbr>isMinSignedValue();<br>
> + return CV->getElementAsInteger(0) ==<br>
> + -(uint64_t)minIntN(Ty-><wbr>getIntegerBitWidth());<br>
<br>
Should we add a getElementAsAPInt instead to avoid having to manually<br>
work out isMinSignedValue() and friends?<br></blockquote><div><br></div><div>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...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + }<br>
> + }<br>
><br>
> return false;<br>
> }<br>
> @@ -179,9 +197,15 @@<br>
> return Splat->isNotMinSignedValue();<br>
><br>
> // Check for constant vectors which are splats of INT_MIN values.<br>
> - if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this))<br>
> - if (Constant *Splat = CV->getSplatValue())<br>
> - return Splat->isNotMinSignedValue();<br>
> + if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(<wbr>this)) {<br>
> + if (CV->isSplat()) {<br>
> + Type *Ty = CV->getElementType();<br>
> + if (Ty->isFloatingPointTy())<br>
> + return !CV->getElementAsAPFloat(0).<wbr>bitcastToAPInt().<wbr>isMinSignedValue();<br>
> + return CV->getElementAsInteger(0) !=<br>
> + -(uint64_t)minIntN(Ty-><wbr>getIntegerBitWidth());<br>
> + }<br>
> + }<br>
><br>
> // It *may* contain INT_MIN, we can't tell.<br>
> return false;<br>
> @@ -2627,17 +2651,21 @@<br>
> return Str.drop_back().find(0) == StringRef::npos;<br>
> }<br>
><br>
> -Constant *ConstantDataVector::<wbr>getSplatValue() const {<br>
> +bool ConstantDataVector::isSplat() const {<br>
> const char *Base = getRawDataValues().data();<br>
><br>
> // Compare elements 1+ to the 0'th element.<br>
> unsigned EltSize = getElementByteSize();<br>
> for (unsigned i = 1, e = getNumElements(); i != e; ++i)<br>
> if (memcmp(Base, Base+i*EltSize, EltSize))<br>
> - return nullptr;<br>
> + return false;<br>
><br>
> + return true;<br>
> +}<br>
> +<br>
> +Constant *ConstantDataVector::<wbr>getSplatValue() const {<br>
> // If they're all the same, return the 0th one as a representative.<br>
> - return getElementAsConstant(0);<br>
> + return isSplat() ? getElementAsConstant(0) : nullptr;<br>
> }<br>
><br>
> //===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
> Index: include/llvm/IR/Constants.h<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- include/llvm/IR/Constants.h<br>
> +++ include/llvm/IR/Constants.h<br>
> @@ -761,6 +761,10 @@<br>
> /// i32/i64/float/double) and must be a ConstantFP or ConstantInt.<br>
> static Constant *getSplat(unsigned NumElts, Constant *Elt);<br>
><br>
> + /// Returns true if this is a splat constant, meaning that all elements have<br>
> + /// the same value.<br>
> + bool isSplat() const;<br>
> +<br>
> /// If this is a splat constant, meaning that all of the elements have the<br>
> /// same value, return that value. Otherwise return NULL.<br>
> Constant *getSplatValue() const;<br>
</blockquote></div><br></div></div>