[PATCH] D11146: [ValueTracking] Add support for floating-point minnum and maxnum

[Hal Finkel]

----- Original Message -----
> From: "James Molloy" <james at jamesmolloy.co.uk>
> To: reviews+D11146+public+461b5467ed880e9d at reviews.llvm.org, "james molloy" <james.molloy at arm.com>, hfinkel at anl.gov,
> "t p northover" <t.p.northover at gmail.com>, "david majnemer" <david.majnemer at gmail.com>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Sunday, July 26, 2015 9:02:56 AM
> Subject: Re: [PATCH] D11146: [ValueTracking] Add support for floating-point minnum and maxnum
> 
> 
> Hi Hal,
> 
> 
> Sure, I'll apply those changes. Just to clarify, when you say "this
> is sad" - are you referring to the deliberate ambiguity in the
> standard or the conservativeness of the code I've written?

The standard, which, I presume, is not your fault.

 -Hal

> 
> 
> Cheers,
> 
> 
> James
> 
> 
> On Sun, 26 Jul 2015 at 14:10 hfinkel at anl.gov < hfinkel at anl.gov >
> wrote:
> 
> 
> hfinkel added inline comments.
> 
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:3345
> @@ +3344,3 @@
> + // (0.0 <= -0.0) ? 0.0 : -0.0 // Returns 0.0
> + // minNum(0.0, -0.0) // May return -0.0 or 0.0 (IEEE 754-2008
> 5.3.1)
> + // Therefore we behave conservatively and only proceed if at least
> one of the
> ----------------
> I feel compelled to say that this is sad. The relevant text for
> minNum/maxNum even notes, "this means results might differ among
> implementations", and this is the only place I can find in the
> standard that says that.
> 
> 
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:3409
> @@ +3408,3 @@
> +
> + // (icmp X, Y) ? X : Y
> + if (TrueVal == CmpLHS && FalseVal == CmpRHS) {
> ----------------
> This comment should now say icmp/fcmp (or similar).
> 
> ================
> Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2277
> @@ -2276,3 +2276,3 @@
> Value *LHS, *RHS;
> - SelectPatternFlavor SPF = matchSelectPattern(const_cast<User*>(&I),
> LHS, RHS);
> + SelectPatternFlavor SPF = matchSelectPattern(const_cast<User*>(&I),
> LHS, RHS).Flavor;
> ISD::NodeType Opc = ISD::DELETED_NODE;
> ----------------
> Line is too long.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1312
> @@ +1311,3 @@
> + // - but only if this isn't part of a min/max operation, else we'll
> + // ruin min/max canonical form.
> + Value *LHS, *RHS;
> ----------------
> What is the canonical form? I think it would be helpful to say (or
> refer to somewhere else that says).
> 
> 
> Repository:
> rL LLVM
> 
> http://reviews.llvm.org/D11146
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory