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

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added a comment.

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

Oh, right. I misunderstood what's doing in these patches and thought we can add metadata to any intrinsics by CGFPOptionsRAII now. :-)

> If a relevant #pragma is used then without this change the metadata ...

Yeah, I understand it now. Thank you! But why we still have the wrong `maytrap` with this patch?

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

Brilliant!

> If use of the constrained intrinsics will cause breakage of the target-specific clang builtins then that's important to know and to fix.

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 at: clang/lib/CodeGen/CGBuiltin.cpp:12268
+  case X86::BI__builtin_ia32_cvtqq2pd512_mask: {
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(*this, E);
     return EmitX86ConvertIntToFp(*this, Ops, /*IsSigned*/true);
----------------
Maybe better to move it into these Emit* functions?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14093
       // exception behavior under strict FP.
+      // NOTE: If strict FP does ever go through here a CGFPOptionsRAII
+      // object will be required.
----------------
It can't go here because of line 14037. But the comment seems good here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94614