[PATCH] D92596: [FPEnv] Correct constrained metadata in fp16-ops-strict.c

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

kpn added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2992
     // floating point environment in the loop.
+//XXX true?
     llvm::BasicBlock *startBB = Builder.GetInsertBlock();
----------------
mibintc wrote:
> did you mean to leave this here? (blame shows the fixme comment dates from 2012)
I was hoping someone knew what "floating point environment" is relevant here. From reading the commit message it doesn't sound like it matters to us, but I thought I'd flag it anyway.

No, I wasn't planning on committing with this comment.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3006
 
+  CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, OpInfo.FPFeatures);
   SourceLocation Loc = E->getExprLoc();
----------------
mibintc wrote:
> What's the rule to follow about when we need to FPOptsRAII? 
It is used on the border between code that has the AST node and code that doesn't. If any code below this point might use the constrained floating point intrinsics then the FPOptsRAII is needed.

Sometimes this border is at a call to the IRBuilder. Sometimes it's buried elsewhere. The hope is that by having the location be defined there we can at some point audit to verify we have it everywhere we should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92596