Page MenuHomePhabricator

add fast-math-flags to 'call' instructions (PR21290)
ClosedPublic

Authored by spatel on Nov 16 2015, 8:27 AM.

Details

Summary

This patch adds optional fast-math-flags (the same that apply to fmul/fadd/fsub/fdiv/frem/fcmp) to call instructions in IR. Follow-up patches would use these flags in LibCallSimplifier, add support to clang, and extend FMF to the DAG for calls.

A motivating example is something like this:

%y = fmul fast float %x, %x
%z = tail call float @sqrtf(float %y)

We'd like to be able to optimize sqrt(x*x) into fabs(x). We do this today using a function-wide attribute for unsafe-math, but we really want to trigger on the instructions themselves:

%z = tail call fast float @sqrtf(float %y)

The code changes and tests are based on the recent commits that added "notail":
http://reviews.llvm.org/rL252368

and added FMF to fcmp:
http://reviews.llvm.org/rL241901

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 40288.Nov 16 2015, 8:27 AM
spatel retitled this revision from to add fast-math-flags to 'call' instructions (PR21290).
spatel updated this object.
spatel added reviewers: jmolloy, ahatanak, hfinkel.
spatel added a subscriber: llvm-commits.
davide added a subscriber: davide.Nov 16 2015, 8:51 AM

Function-level attributes fall apart when code gets mixed by LTO; please see PR21290 for more details:
https://llvm.org/bugs/show_bug.cgi?id=21290

See also comments here:
http://reviews.llvm.org/D7257

See also comments here:
http://reviews.llvm.org/D7257

Disregard that one - that was just about fcmp.

ahatanak added inline comments.Nov 16 2015, 1:41 PM
lib/AsmParser/LLParser.cpp
5647 ↗(On Diff #40288)

Is it possible to commit the 'notail' change in a separate patch (and thank you for catching this)?

5678 ↗(On Diff #40288)

Are there cases where you want to optimize calls to functions that take floating point arguments but don't have floating point return types?

spatel added inline comments.Nov 16 2015, 2:26 PM
lib/AsmParser/LLParser.cpp
5647 ↗(On Diff #40288)

Sure - will do that first and update this patch.

5678 ↗(On Diff #40288)

I thought about that too, however, if we use this definition, then we do not have to redefine "isa<FPMathOperator>" in this patch.

I don't know if that is a reasonable justification by itself, but I think we can expand the definition of FPMathOperator in a separate patch if we do decide that FMF should apply to more types of calls.

My 2 ¢:

It seems more natural to me to phrase these as normal string attributes at the point of the call and construct an FMF from a CallSite but I can see arguments for and against both designs.

spatel added a subscriber: t.p.northover.

My 2 ¢:

It seems more natural to me to phrase these as normal string attributes at the point of the call and construct an FMF from a CallSite but I can see arguments for and against both designs.

I thought about it some more, looked into the implementation, and convinced myself that the attribute solution isn't as good.

  1. We already have some of the function-level FMF attributes here: http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/include/llvm/IR/Attributes.td but they are 'StrBoolAttr' rather than the 'EnumAttr' that I think we want. The names don't match. There would be some duplication/ugliness here at the least.
  2. This is the big one: we'd need to update independent chunks of code any time FMF changes. This scares me because I'm still hoping to get advanced reciprocal estimate functionality ( http://reviews.llvm.org/rL239001 , http://reviews.llvm.org/rL239536 ) into instruction-level FMF. Those settings don't exist at all in IR currently, so in an LTO build, they simply disappear. I think the same thing happens with FMAs. The likelihood that FMF internals require changes seems high, so having a unified FMF interface for all instructions is the better solution.
spatel updated this revision to Diff 40686.Nov 19 2015, 11:53 AM

Patch updated:
As suggested by Akira, I added the 'notail' comment and updated the error message in ParseCall() in r253580 since they are independent from this patch.

The likelihood that FMF internals require changes seems high, so having a unified FMF interface for all instructions is the better solution.

As evidence that FMF is evolving, there's currently a patch under review that would add more flags to support observing the FENV:
http://reviews.llvm.org/D14067

hfinkel accepted this revision.Dec 10 2015, 2:15 PM
hfinkel edited edge metadata.

A few minor things, and with these changes, LGTM.

docs/LangRef.rst
8453 ↗(On Diff #40686)

I find this confusing; we should just use [fast-math flags]* instead of [fmf] as we do elsewhere.

8510 ↗(On Diff #40686)

Adjust for [fmf] -> [fast-math flags]*

8512 ↗(On Diff #40686)

We must document here that these flags are valid only for calls with a floating-point scalar or vector return type.

lib/AsmParser/LLParser.cpp
5677 ↗(On Diff #40686)

I think it would be better to say how the call was invalid (and, minus the fmfs, the call itself may be fine). How about:

return Error(CallLoc, "fast-math-flags specified for call without floating-point scalar or vector return type");
lib/Bitcode/Reader/BitcodeReader.cpp
5108 ↗(On Diff #40686)

Match this error text to the text in lib/AsmParser/LLParser.cpp.

This revision is now accepted and ready to land.Dec 10 2015, 2:15 PM
spatel marked 5 inline comments as done.Dec 14 2015, 1:43 PM
spatel updated this revision to Diff 42764.Dec 14 2015, 1:47 PM
spatel edited edge metadata.

Patch updated based on Hal's feedback and rebased (one more existing test for SimplifyLibCalls now has the 'fast' flag).

This revision was automatically updated to reflect the committed changes.

I wonder about the design of this, especially since Chris (and Chandler) rejected it in the past: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121217/159446.html ?

(note there might be a fundamental difference I didn't see skimming through this)