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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to