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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
