[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3283
+Sets the current rounding mode. Encoding of the returned values is
+same as the result of FLT_ROUNDS, specified by C standard:
+0  - toward zero
----------------
"Sets the current floating-point rounding mode.  The parameter uses the same values specified for ``FLT_ROUNDS`` by the C standard:
...
The effect of passing some other value to this builtin is implementation-defined."


================
Comment at: clang/docs/LanguageExtensions.rst:3293
+This builtin is converted to llvm.set.rounding intrinsic in LLVM IR level
+and not all targets support this intrinsic, so only x86 and arm targets
+support this builtin. Since this builtin changes default floating-point
----------------
sepavloff wrote:
> If some other target start supporting `llvm.set_rounding` this statement becomes wrong. Maybe just 'some targets'?
The manual should document when a specific builtin is restricted to specific targets.  If someone adds support for the builtin on new targets, they should update the manual.  Think about it from a user's perspective: they should be able to check what targets support this builtin without having to figure out that the builtin turns into a specific intrinsic and then crawl around in the LLVM source code to figure out which targets implement that intrinsic.

With that said, please do not refer to LLVM-level details in the Clang documentation.  Users shouldn't have to care that the builtin is lowered to such-and-such LLVM intrinsic.  Even if they did, that's not a hard guarantee we want to make anyway — we document high-level semantics, not the fine details of the intermediate representation coming out of the frontend.

Also, please do not cross-reference the LLVM documentation.  The Clang documentation is user-facing and has a much, much wider potential audience than the LLVM documentation, which is focused on implementors.  People should be able to use the compiler without worrying about backend details.  If we have to repeat some information between the two places, that's fine.


================
Comment at: clang/docs/LanguageExtensions.rst:3295
+support this builtin. Since this builtin changes default floating-point
+environment, ``#pragma STDC FENV_ACCESS ON`` is required.
+
----------------
sepavloff wrote:
> Not necessary. Options `-ffp-model=strict` or `-frounding-math` also can be used. I would remove this statement.
It is appropriate to at least say that not all modes permit changing the default floating-point environment and cross-reference the appropriate place in the Clang documentation where we talk about that in more detail.  If we don't have such a place already, this is the time to add it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146188/new/

https://reviews.llvm.org/D146188