Currently clang is not correctly retrieving from the AST the metadata for constrained FP builtins. This patch fixes that for the X86 specific builtins.
For previous work in this vein see D92122 for example.
Paths
| Differential D94614
[FPEnv][X86] Platform builtins edition: clang should get from the AST the metadata for constrained FP builtins ClosedPublic Authored by kpn on Jan 13 2021, 10:04 AM.
Details Summary Currently clang is not correctly retrieving from the AST the metadata for constrained FP builtins. This patch fixes that for the X86 specific builtins. For previous work in this vein see D92122 for example.
Diff Detail
Event TimelineComment Actions Hi Kevin, what's the intention of adding constrained FP metadata for target dependent builtins? I believe the middle-end passes always ignore these builtins. What's more, will it imply user these builtins have different behaviors under different FP model? But it's not true for most platform builtins, since they are just representative of given instructions. Comment Actions 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. Comment Actions
Oh, right. I misunderstood what's doing in these patches and thought we can add metadata to any intrinsics by CGFPOptionsRAII now. :-)
Yeah, I understand it now. Thank you! But why we still have the wrong maytrap with this patch?
Brilliant!
I had a look at these changes and didn't find anything will cause breakage for now. I'm still not sure if we need to teach target-specific clang builtins to respect strict FP or not. But it has nothing to do with this patch.
Comment Actions Update for review comments: Move uses of CGFPOptionsRAII lower and closer to where they are needed. This should be less error prone as well. This revision is now accepted and ready to land.Jan 26 2021, 4:53 AM Closed by commit rG81b69879c946: [FPEnv][X86] Platform builtins edition: clang should get from the AST the… (authored by kpn). · Explain WhyFeb 3 2021, 8:49 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 321108 clang/lib/CodeGen/CGBuiltin.cpp
clang/test/CodeGen/X86/avx-builtins-constrained-cmp.c
clang/test/CodeGen/X86/avx512dq-builtins-constrained.c
clang/test/CodeGen/X86/avx512f-builtins-constrained.c
clang/test/CodeGen/X86/fma-builtins-constrained.c
clang/test/CodeGen/X86/sse-builtins-constrained.c
|
Maybe better to move it into these Emit* functions?