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.

Download Raw Diff

Details

Reviewers

efriedma
jfb
reames
scanon
lebedev.ri
spatel

Commits

rG6b3749f6968f: [LangRef] Clarify absence of rounding guarantees for fmuladd.
rL372892: [LangRef] Clarify absence of rounding guarantees for fmuladd.

Summary

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

Diff Detail

Repository

rG LLVM Github Monorepo

Build Status

Buildable 38132
Build 38131: arc lint + arc unit

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

fhahn mentioned this in D67434: [InstCombine] Limit FMul constant folding for fma simplifications..Sep 13 2019, 7:37 AM

Harbormaster completed remote builds in B38102: Diff 220099.Sep 13 2019, 7:38 AM

Sounds good to me

reames added inline comments.Sep 13 2019, 9:11 AM

llvm/docs/LangRef.rst
13953–13954	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. "
13954–13959	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
13953–13954	Sounds good, thanks! Do you think that clarification is enough, without the additional sentence?
13954–13959	Do you mean when reading the sentence or with sphinx?

reames added inline comments.Sep 13 2019, 6:27 PM

llvm/docs/LangRef.rst
13953–13954	Yes. LGTM.
13954–13959	When reading.

Updated with @reames' suggestion, thanks!

Harbormaster completed remote builds in B38132: Diff 220216.Sep 14 2019, 8:00 AM

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
13953–13954	Done, thanks!

Sounds good to me.

@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	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."

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

llvm/docs/LangRef.rst
13956	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

In D67552#1680844, @spatel wrote:

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.

In D67552#1681287, @fhahn wrote:

In D67552#1680844, @spatel wrote:

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.

Closed by commit rL372892: [LangRef] Clarify absence of rounding guarantees for fmuladd. (authored by fhahn). · Explain WhySep 25 2019, 9:08 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

llvm/

docs/

LangRef.rst

12 lines

Diff 220216

llvm/docs/LangRef.rst

This file is larger than 256 KB, so syntax highlighting is disabled by default.

	Show First 20 Lines • Show All 13,944 Lines • ▼ Show 20 Lines
	Semantics:			Semantics:
	""""""""""			""""""""""

	The expression:			The expression:

	::			::

	%0 = call float @llvm.fmuladd.f32(%a, %b, %c)			%0 = call float @llvm.fmuladd.f32(%a, %b, %c)

	is equivalent to the expression a \* b + c, except that rounding will			is equivalent to the expression a \* b + c, except that it is unspecified
				reamesUnsubmitted Not Done Reply Inline Actions 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. " reames: Please change: "is equivalent to the expression a \* b + c, except that rounding will not be…
				fhahnAuthorUnsubmitted Done Reply Inline Actions Sounds good, thanks! Do you think that clarification is enough, without the additional sentence? fhahn: Sounds good, thanks! Do you think that clarification is enough, without the additional sentence?
				reamesUnsubmitted Not Done Reply Inline Actions Yes. LGTM. reames: Yes. LGTM.
				fhahnAuthorUnsubmitted Done Reply Inline Actions Done, thanks! fhahn: Done, thanks!
	not be performed between the multiplication and addition steps if the			whether rounding will be performed between the multiplication and addition
	code generator fuses the operations. Fusion is not guaranteed, even if			steps. Fusion is not guaranteed, even if the target platform supports it.
				spatelUnsubmitted Not Done Reply Inline Actions 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: Worth adding a sentence to explain why we have this intrinsic at all? Something like: "The…
				spatelUnsubmitted Not Done Reply Inline Actions On 2nd thought, strike that suggestion - it's redundant with the text in the 'Overview' above this. spatel: On 2nd thought, strike that suggestion - it's redundant with the text in the 'Overview' above…
	the target platform supports it. If a fused multiply-add is required, the			If a fused multiply-add is required, the corresponding
	corresponding :ref:`llvm.fma <int_fma>` intrinsic function should be used			:ref:`llvm.fma <int_fma>` intrinsic function should be used instead.
	instead. This never sets errno, just as '``llvm.fma.*``'.			This never sets errno, just as '``llvm.fma.*``'.
				reamesUnsubmitted Not Done Reply Inline Actions Your added sentence does not parse for me. reames: Your added sentence does not parse for me.
				fhahnAuthorUnsubmitted Done Reply Inline Actions Do you mean when reading the sentence or with sphinx? fhahn: Do you mean when reading the sentence or with sphinx?
				reamesUnsubmitted Not Done Reply Inline Actions When reading. reames: When reading.

	Examples:			Examples:
	"""""""""			"""""""""

	.. code-block:: llvm			.. code-block:: llvm

	%r2 = call float @llvm.fmuladd.f32(float %a, float %b, float %c) ; yields float:r2 = (a * b) + c			%r2 = call float @llvm.fmuladd.f32(float %a, float %b, float %c) ; yields float:r2 = (a * b) + c

	▲ Show 20 Lines • Show All 3,726 Lines • Show Last 20 Lines