[LLVMdev] Representing -ffast-math at the IR level

[Duncan Sands]

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>