[PATCH] D146408: [clang][Interp] Start supporting complex types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/Context.cpp:84-85
 
+  if (T->isAnyComplexType())
+    return std::nullopt;
+
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Hmmm, this seems surprising to me, I would have assumed that _Complex needed to be a primitive type so that we could perform typical math on it rather than treat it as a pair of values.
> I was going to add special opcodes for this case (or add //one// opcode for complex/vector/bitint/... things to do everything?). 
> 
> If we don't do it this way we'd have to add `isAnyComplexType()` checks everywhere so we can figure out if we need to look at one or two values, don't we?
Hmm I think it depends on whether we consider the type to be primitive or not, maybe? I can see why it's not primitive -- it's comprised of two other primitive types. So that could be reasonable. But it's also primitive in that you need both of those values in order for the type to be meaningful for any operation (e.g., we don't want to accidentally split the values; we're always going to want to pass a complex value as "one thing", right). Buttttttt then again, if we treat it as not being primitive, then perhaps that gives us more optimization opportunities around things like "is an array of complex values an array of structs or a struct of arrays" kind of situations where we can decide to handle it differently depending on the situation.

So I guess I'm not certain of whether this is wrong or right.


================
Comment at: clang/test/AST/Interp/complex.cpp:7
+
+constexpr _Complex double z1 = {1.0, 2.0};
+static_assert(__real(z1) == 1.0);
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Can you add tests for `_Complex int` as well? (That's also covered by `isAnyComplexType()`)
> There's a `FIXME` comment above in `EvalEmitter.cpp` so returning those doesn't work yet (but is otherwise not problematic I think.)
Hmmm, why the FIXME instead of just making the complex int type? (Alternatively, should we not use `isAnyComplexType()` and wait to add complex int support later?)


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

https://reviews.llvm.org/D146408