[PATCH] D94614: [FPEnv][X86] Platform builtins edition: clang should get from the AST the metadata for constrained FP builtins

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

This doesn't add metadata to llvm intrinsics that are not constrained.

The metadata is added by the IRBuilder when the IRBuilder is used to add constrained intrinsics. When adding non-constrained intrinsics that have a direct mapping to constrained intrinsics, and constrained mode is enabled, the IRBuilder will add a constrained intrinsic instead of the requested non-constrained intrinsic. This keeps the changes to the front-end smaller.

But the metadata is _only_ added when the IRBuilder knows it is adding a constrained intrinsic. So we're not adding anything to any of the other llvm intrinsics. The target-specific llvm intrinsics are no exception to this rule.

Some of the clang builtins get lowered in whole or in part to llvm instructions (or constrained FP calls). Currently they use the metadata specified by the command line arguments. This change causes them to pick up the metadata that the AST says should be used. If no #pragma was used then the AST will be carrying the metadata from the command line. If a relevant #pragma is used then without this change the metadata in the #pragma's scope will be wrong. I'm testing for this by having the RUN lines specify "maytrap" but then use the #pragma float_control to switch to "fpexcept.strict".

The IRBuilder's idea of the current metadata is updated by CGFPOptionsRAII. That's why you see it used right around places where we leave the clang AST and call into code that doesn't have access to the AST. Like the IRBuilder.

A similar issue exists with the rounding-mode metadata. But both rounding and exception behavior are set by CGFPOptionsRAII, and if that changes then other tests already in the tree will fail. So I'm not bothering with rounding metadata testing here.

It's true that the middle-end optimizers know nothing about the constrained intrinsics. Today. I plan on changing that in the near future.

If use of the constrained intrinsics will cause breakage of the target-specific clang builtins then that's important to know and to fix. And I don't know all the targets well enough to know if that is a problem. So I'm going around asking target-specific developers to take another look and make sure we aren't setting ourselves up for problems later by having these builtins lower with the correct metadata for this point in the AST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94614