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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
