[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:620
+    //unsigned FPFeatures : 21;
+    unsigned FPFeatures : 32;
   };
----------------
riccibruno wrote:
> mibintc wrote:
> > This is a temporary change, I know you don't want me to change the assert to allow a wider size than 8. So if this information is really needed (as I mentioned above it's not being used in Sema or Codegen) then alternatives are
> > 1. rewrite the == comparison using BinaryOperator
> > 2. put the FPFeatures Override into TrailingStorage (i think i will need implementation guidance if this is the path to go)
> What do you mean by `rewrite the == comparison using BinaryOperator`? If you mean representing overloaded `operator==`'s by `BinaryOperator`, then that's certainly not right because `BinaryOperator`s only represent binary builtin operators.
> 
> You will not be able to use `TrailingObjects` with `CXXOperatorCallExpr` because `CallExpr` stores its arguments after the object containing the `CallExpr` sub-object. You could manually stash it after `CallExpr`'s arguments, but that would be pretty ugly.
> 
> Do you really need 32-bits worth of FP options? I see from `FPOptions.def` that you need 14 bits, so it should fit with room to spare.
Bruno's close to right.  The key question here is what node we use to represent dependent operators that might instantiate to a builtin floating-point operator, and the answer for binary operators (based on the code in `Sema::CreateOverloadedBinOp`) is that we use `CXXOperatorCallExpr` if lexical lookup found any matching operators (because we need some way to represent the operators, which `BinaryOperator` doesn't have) and otherwise we use `BinaryOperator`.

So, first, to create a test that depends on this, you need a dependent operator for which there is an overload in lexical scope, and then it needs to instantiate to just do a builtin operation, and this needs to be sensitive to the currently-active FP settings.  So maybe something like:

```
struct Distance {};
Distance operator+(Distance, Distance);

template <class T> T add(T lhs, T rhs) {
#pragma STDC FENV_ACCESS on // or whatever pragma you like, doesn't have to be local, just has to override current settings and be different from the default
  return lhs + rhs;
}

float test() {
  return add(1.0f, 2.0f);
}
```

Second, how to represent this: while I think there would be some benefits from introducing a new `Expr` subclass that we always use to represent a dependent operator, I think it's fine (and certainly easier) to just stick with `CXXOperatorCallExpr` like we do now.  Given that, you can either follow Bruno's advice and use fewer bits — which is probably a fairly short-term fix, since tracking almost any extra state would get us into trouble — or figure out some other way to store these.

Honestly, it's not the end of the world if `CXXOperatorCallExpr` just stores an `FPOptionsOverride` as an ordinary field — it just means we're not taking advantage of the fact that most contexts don't have overrides.  Most instances of this operator are in dependent code where we can't know that we won't instantiate to a builtin operator, so there's no way to avoid storing the overrides if there are any.

If we do want to store FP features in trailing storage, then we're going to have to worry about the fact that `CallExpr` already has trailing storage.  I think the right approach in this case is to have `CallExpr` generally account for the overrides in its trailing storage and then, dynamically, we know that that only actually happens with `CXXOperatorCallExpr` — all the other factories pass down empty overrides.  One specific advantage of this is that it sets the stage for storing overrides on calls to builtins; we'd just need Sema to pass overrides down when it's building a builtin call.  (We can rely on this even in templates because you can't invoke a builtin indirectly or by instantiation.)


================
Comment at: clang/include/clang/Basic/LangOptions.h:455
+  llvm::errs() << "\n RoundingMode " <<
+        static_cast<unsigned>(getRoundingMode());
+  llvm::errs() << "\n FPExceptionMode " << getFPExceptionMode();
----------------
riccibruno wrote:
> mibintc wrote:
> > Using #define OPTION trick would be preferrable except there is a compilation failure because something funny about RoundingMode--need the cast. doubtless there is fix for that
> Yes it would be preferable and it should not be in a header too.
> 
> You need the cast because `RoundingMode` is a scoped enumeration, which does not have the implicit conversions of unscoped enumerations.
Putting a cast to `unsigned` in the `#define OPTION` should be fine; if any of these fields outgrow that, we probably need to reconsider the representation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81869