This is an archive of the discontinued LLVM Phabricator instance.

[LangRef] Clarify absence of rounding guarantees for fmuladd.
ClosedPublic

Authored by fhahn on Sep 13 2019, 7:33 AM.

Details

Summary

During the review of D67434, it was recommended to make fmuladd's
behavior more explicit. D67434 depends on this interpretation.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Sep 13 2019, 7:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 7:33 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript

Sounds good to me

reames added inline comments.Sep 13 2019, 9:11 AM
llvm/docs/LangRef.rst
13954 ↗(On Diff #220099)

Please change:
"is equivalent to the expression a \* b + c, except that rounding will
not be performed between the multiplication and addition steps if the
code generator fuses the operations. "

To:
"is equivalent to the expression a \* b + c, except that it is unspecified whether rounding will be performed between the multiplication and addition steps. "

13956 ↗(On Diff #220099)

Your added sentence does not parse for me.

fhahn marked 2 inline comments as done.Sep 13 2019, 12:26 PM
fhahn added inline comments.
llvm/docs/LangRef.rst
13954 ↗(On Diff #220099)

Sounds good, thanks! Do you think that clarification is enough, without the additional sentence?

13956 ↗(On Diff #220099)

Do you mean when reading the sentence or with sphinx?

reames added inline comments.Sep 13 2019, 6:27 PM
llvm/docs/LangRef.rst
13954 ↗(On Diff #220099)

Yes. LGTM.

13956 ↗(On Diff #220099)

When reading.

fhahn updated this revision to Diff 220216.Sep 14 2019, 8:00 AM

Updated with @reames' suggestion, thanks!

fhahn retitled this revision from [LangRef] Clarify fmuladd(a, b, c) can be treated as fadd (fmul a, b), c). to [LangRef] Clarify absence of rounding guarantees for fmuladd..Sep 14 2019, 8:06 AM
fhahn marked an inline comment as done.
fhahn added inline comments.
llvm/docs/LangRef.rst
13954 ↗(On Diff #220099)

Done, thanks!

Sounds good to me.

fhahn added a comment.Sep 17 2019, 1:15 PM

@reames I think this should be good to go in now?

spatel added inline comments.Sep 24 2019, 7:30 AM
llvm/docs/LangRef.rst
13956 ↗(On Diff #220216)

Worth adding a sentence to explain *why* we have this intrinsic at all? Something like:
"The optional fusion gives flexibility to the IR optimizer and code generator to select the best-performing target code sequence."

spatel accepted this revision.Sep 24 2019, 7:35 AM

LGTM (though I think we're waiting for @reames).

llvm/docs/LangRef.rst
13956 ↗(On Diff #220216)

On 2nd thought, strike that suggestion - it's redundant with the text in the 'Overview' above this.

This revision is now accepted and ready to land.Sep 24 2019, 7:35 AM
fhahn added a comment.Sep 24 2019, 1:27 PM

LGTM (though I think we're waiting for @reames).

Thanks!. From the previous comments my understanding is that the re-phrasing looks good to him, but I'll wait for a few more days, in case there are additional concerns.

LGTM (though I think we're waiting for @reames).

Thanks!. From the previous comments my understanding is that the re-phrasing looks good to him, but I'll wait for a few more days, in case there are additional concerns.

No need to wait.

In general, unless I ask explicitly to wait or indicate a desire to see a future version, you don't need to wait for me to circle back if someone else gave an LGTM.

This revision was automatically updated to reflect the committed changes.