Hi Chandler,
> Minor comments:
> + if (!Accuracy)
> + // If it's not a floating point number then it must be
'fast'.
> + return HUGE_VALF;
>
> Can we add an assert instead of a comment? It's just as documenting and
will
> catch any goofs.
Done.
>
> + // If it's not a floating point number then it must be
'fast'.
> + return !isa<ConstantFP>(MD->getOperand(0));
>
> Here as well.
>
> + if (ConstantFP *CFP0 = dyn_cast_or_null<ConstantFP>(Op0)) {
> + APFloat Accuracy = CFP0->getValueAPF();
> + Assert1(Accuracy.isNormal() && !Accuracy.isNegative(),
> + "fpmath accuracy not a positive number!", &I);
>
> To be pedantic for a moment, zero is not a positive number.
Zero is not allowed. The isNormal call will return false for zero.
What about asserting> these individually to give us more clear asserts if they fire? That also
makes
> the string easier to write: "fpmath accuracy is a negative
number!".
It will fire on: zero, negative numbers, NaN, +-infinity. Personally I reckon
"fpmath accuracy not a positive number!" is reasonable for all of
these.
>
> + /// SetDefaultFPMathTag - Set the floating point math metadata to be
used.
> + void SetDefaultFPMathTag(MDNode *FPMathTag) { DefaultFPMathTag =
FPMathTag; }
>
> This should be 'setDefault...' much like 'getDefault...'
above.
The rest of IRBuilder uses a capital S in its setters, so I was just trying to
be consistent here.
>
> + Instruction *AddFPMathTag(Instruction *I, MDNode *FPMathTag) const {
>
> Another bad case, but I think this instruction is gone...
It still exists, and is also capitalized like that for consistency with the rest
of IRBuilder.
>
> + MDString *GetFastString() const {
> + return CreateString("fast");
> + }
>
> 'getFastString'.
OK, done - same for the others that are not in IRBuilder.
> + /// CreateFastFPMath - Return metadata with appropriate settings for
'fast
> + /// math'.
>
> I would prefer the more modern doxygen style:
>
> /// \brief Return metadata ...
>
> + MDNode *CreateFastFPMath() {
>
> Capitalization.
>
> The capitalization and doxygen style comments apply to the next function as
well.
>
>
> Both the Clang and DragonEgg patches look good, but both need test cases.
=]
Yes, I'm working on those as well. See attached patch for the other
changes.
It now also includes a unit test.
Ciao, Duncan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fastm.diff
Type: text/x-patch
Size: 26765 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120416/c9a2ac15/attachment.bin>