This is an archive of the discontinued LLVM Phabricator instance.

Add support for fast-math flags to the FCmp instruction.
ClosedPublic

Authored by jmolloy on May 15 2015, 6:58 AM.

Details

Summary

FCmp behaves a lot like a floating-point binary operator in many ways,
and can benefit from fast-math information. Flags such as nsz and nnan
can affect if this fcmp (in combination with a select) can be treated
as a fminnum/fmaxnum operation.

This adds backwards-compatible bitcode support, IR parsing and writing,
LangRef changes and IRBuilder changes. I'll need to audit InstSimplify
and InstCombine in a followup to find places where flags should be
copied.

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy updated this revision to Diff 25866.May 15 2015, 6:58 AM
jmolloy retitled this revision from to Add support for fast-math flags to the FCmp instruction..
jmolloy updated this object.
jmolloy edited the test plan for this revision. (Show Details)
jmolloy added reviewers: hfinkel, majnemer, chandlerc.
jmolloy set the repository for this revision to rL LLVM.
jmolloy added a subscriber: Unknown Object (MLST).
majnemer added inline comments.May 15 2015, 1:52 PM
docs/LangRef.rst
6978–6981

We really need to say more here about the semantics of fast math flags in this position.

For example, does arcp on an fcmp imply that i can take:

%div = fdiv double 1.000000e+00, %a
%div1 = fdiv double 1.000000e+00, %b
%cmp = fcmp arcp olt double %div, %div1

and replace it with:

%cmp = fcmp ogt double %a, %b

?

hfinkel added inline comments.May 18 2015, 3:25 PM
docs/LangRef.rst
6978–6981

I agree. FWIW, I'd say the answer should be no. We should require arcp on the fdivs for that.

jmolloy updated this revision to Diff 27110.Jun 4 2015, 5:33 AM

Hi Hal, David,

I've updated the patch to explicitly say that the only flags that should change semantics are those that allow assumptions about input arguments (not transforms).

Sorry about the long round-trip on this, I went on vacation for 2 weeks!

Cheers,

James

spatel added a subscriber: spatel.Jun 4 2015, 1:31 PM
hfinkel accepted this revision.Jun 22 2015, 4:39 PM
hfinkel edited edge metadata.

LGTM.

docs/LangRef.rst
6984

I'd rather word this as, "that allow assumptions to be made about the values of the input arguments", because implying that there is a mathematical 'domain' that includes NaN might be confusing.

6988

Why are you deleting this statement in this patch? Is this a separate cleanup?

This revision is now accepted and ready to land.Jun 22 2015, 4:39 PM
jmolloy closed this revision.Jul 10 2015, 6:05 AM

Thanks Hal - I've committed this (without the extra text removal; I'll add that as a separate cleanup as you suggest) in r241901.