[PATCH] [complex] Teach Clang to preserve different-type operands to arithmeticoperators where one type is a C complex type, and to emit both the efficientand correct implementation for complex arithmetic according to C11 AnnexG using this extra...

[Richard Smith]

I'd like to see some added tests for constant folding here.

================
Comment at: lib/AST/ExprConstant.cpp:7694-7697
@@ -7694,1 +7693,6 @@
+  assert(E->isRValue() && "Can only evaluate R-values!");
+  assert((E->getType()->isAnyComplexType() ||
+          E->getType()->isRealFloatingType()) &&
+         "Complex expressions can only be composed of complex and real "
+         "floating point types.");
   return ComplexExprEvaluator(Info, Result).Visit(E);
----------------
How do we reach this case? The below handling for real operands calls `EvaluateFloat` rather than `EvaluateComplex`.

================
Comment at: lib/AST/ExprConstant.cpp:7883
@@ +7882,3 @@
+  // the case we can simplify our evaluation strategy.
+  bool LHSReal = false, RHSReal = false;
+
----------------
You should assign `true` to these somewhere =)

================
Comment at: lib/AST/ExprConstant.cpp:7909
@@ -7884,3 +7908,3 @@
 
   assert(Result.isComplexFloat() == RHS.isComplexFloat() &&
          "Invalid operands to binary operator.");
----------------
Maybe `assert(!(LHSReal && RHSReal));` here?

================
Comment at: lib/AST/ExprConstant.cpp:7933
@@ +7932,3 @@
+        Result.getComplexFloatImag() = RHS.getComplexFloatImag();
+        Result.getComplexFloatImag().changeSign();
+      } else if (!RHSReal) {
----------------
Is `changeSign` right here? Should `0 - (0+0i)` be `0+0i` or `0-0i`?

================
Comment at: lib/AST/ExprConstant.cpp:8013-8014
@@ -7967,2 +8012,4 @@
+      if (!RHSReal) {
       Tmp = LHS_r;
       Tmp.multiply(RHS_i, APFloat::rmNearestTiesToEven);
+      }
----------------
Indentation looks off here. Maybe it's just Phab?

================
Comment at: lib/AST/ExprConstant.cpp:8023
@@ +8022,3 @@
+        assert(!RHSReal && "Cannot have both operands be real!");
+        Tmp.changeSign();
+        Res_i = Tmp;
----------------
Does this do the right thing with negative zeros? Eg, `1 / (1 + 0i)` will give `1-0i` but should presumably be `1+0i`.

================
Comment at: lib/CodeGen/CGExprComplex.cpp:556-557
@@ +555,4 @@
+    else
+      ResI = Op.LHS.second ? Op.LHS.second
+                           : Builder.CreateFNeg(Op.RHS.second, "sub.i");
+    assert(ResI && "Only one operand may be real!");
----------------
Same question as before regarding negative zeros.

================
Comment at: lib/CodeGen/CGExprComplex.cpp:593
@@ +592,3 @@
+  } else {
+    assert(Result->getType()->isAggregateType() && "Only vector and aggregate libcall returns are supported!");
+    unsigned ResRIndices[] = {0};
----------------
My 80-col sense is tingling.

================
Comment at: lib/CodeGen/CGExprComplex.cpp:618-619
@@ +617,4 @@
+    if (Op.LHS.second && Op.RHS.second) {
+      // If both operands are complex, delegate to a libcall which works to
+      // prevent underflow and overflow.
+      StringRef LibCallName;
----------------
Do we need to do anything about underflow/overflow in the constant evaluator?

http://reviews.llvm.org/D5698