[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...

[Chandler Carruth]

All comments addressed. Answers to questions below. Fresh patches uploaded and ready for review.

The constant folding is now in good shape I deem it. Took some hacking on APFloat to get there. Added test cases as well. Had fun testing complex division with Wolfram Alpha. =] We seem to get the right answers.

One area I'm not testing heavily is sign propagation in the face of infinities. I'd appreciate if someone (yea, I'm looking at you Steve) could contribute more thorough testing of the sign propagation properties of these operations, and/or tighten or relax the assertions on the real vs. imaginary components to match what numerics folks actually want to enshrine in the constant folder. Hopefully my test case is good enough to make it clear how to extend things going forward.

I also have plans to address both the libcall performance concerns in general and the fast-math concerns in follow-up patches, but I'd like to get a baseline of correctness in first. =]

================
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);
----------------
rsmith wrote:
> How do we reach this case? The below handling for real operands calls `EvaluateFloat` rather than `EvaluateComplex`.
We don't. This was a speculative edit I shouldn't have made.

================
Comment at: lib/AST/ExprConstant.cpp:7883
@@ +7882,3 @@
+  // the case we can simplify our evaluation strategy.
+  bool LHSReal = false, RHSReal = false;
+
----------------
rsmith wrote:
> You should assign `true` to these somewhere =)
Indeed. As you noticed (and I tried but failed to make clear) once I knew I would have to re-implement this from the principled version in our libcalls, I stopped testing it and this was completely broken and wrong.

================
Comment at: lib/AST/ExprConstant.cpp:7909
@@ -7884,3 +7908,3 @@
 
   assert(Result.isComplexFloat() == RHS.isComplexFloat() &&
          "Invalid operands to binary operator.");
----------------
rsmith wrote:
> Maybe `assert(!(LHSReal && RHSReal));` here?
Sure. I had some of that later, but nicer to state it up front.

================
Comment at: lib/AST/ExprConstant.cpp:7933
@@ +7932,3 @@
+        Result.getComplexFloatImag() = RHS.getComplexFloatImag();
+        Result.getComplexFloatImag().changeSign();
+      } else if (!RHSReal) {
----------------
rsmith wrote:
> Is `changeSign` right here? Should `0 - (0+0i)` be `0+0i` or `0-0i`?
Quite surprisingly, I believe this is correct according to the table in G.5.2p2. It stipulates that a complex value subtracted from a real value as x - (u + iv) == (x - u) - iv. Because subtraction is represented as the sum of a negative, especially for complex numbers, this indicates that 'v' must be negated here, even if that would form 0.0 - 0.0i. The 'negative zero' here which is surprising resulting from subtraction of two values with the same sign isn't a big deal because in complex form it just feeds another subtraction.

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

================
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!");
----------------
rsmith wrote:
> Same question as before regarding negative zeros.
Same answer.

================
Comment at: lib/CodeGen/CGExprComplex.cpp:590
@@ +589,3 @@
+  if (Result->getType()->isVectorTy()) {
+  ResR = CGF.Builder.CreateExtractElement(Result, CGF.Builder.getInt32(0));
+  ResI = CGF.Builder.CreateExtractElement(Result, CGF.Builder.getInt32(1));
----------------
hfinkel wrote:
> Indent?
Fixed.

================
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};
----------------
rsmith wrote:
> My 80-col sense is tingling.
Fixed....

BY THE POWER OF CLANG-FORMAT!

================
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;
----------------
rsmith wrote:
> Do we need to do anything about underflow/overflow in the constant evaluator?
Not necessarily. Unlike with integers, such events should (IMO) simply produce the relevant infinity or zero value.

My inclination is that the constant folder should error when a full operation produces a NaN. This would let the complex arithmetic logic shield it from certain NaN conditions, but still bail when we go Too Far.

http://reviews.llvm.org/D5698