[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

Thu Jan 31 15:53:08 PST 2019

rjmccall added inline comments.

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444
   case BO_NE:
+    return Builder.CreateICmpNE(FullLHS, FullRHS);
+  case BO_Mul:
----------------
leonardchan wrote:
> rjmccall wrote:
> > Are padding bits guaranteed zero or unspecified?  Or are we just not really supporting padding bits all the way to IRGen at this time?
> I believe the consensus was leaving them unspecified, so operations that can cause overflow into them would result in undefined behavior.
If I'm understanding you correctly, you're saying that (1) we are assuming that inputs are zero-padded and (2) we are taking advantage of the non-saturating-overflow-is-UB rule to avoid clearing the padding bits after arithmetic.  That's actually what I meant by "guaranteed zero", so we have our labels reversed, but at least we understand each other now.

(I suppose you're thinking of this as "unspecified" because non-saturating arithmetic can leave an unspecified value there.  I think of this as a "guarantee" because it's basically a representational invariant: it's a precondition for correctness that the bit is zero, and all operations guarantee that the bit will be zero in their well-defined cases (but overflow is not well-defined).  Just a difference in perspective, I guess.)

Is this written down somewhere?  Are there targets that use the opposite ABI rule that we might need to support someday?

At any rate, I think it's worth adding a short comment here explaining why it's okay to do a normal comparison despite the possibility of padding bits.  Along those lines, is there any value in communicating to the backend that padded-unsigned comparisons could reasonably be done with either a signed or unsigned comparison?  Are there interesting targets where one or the other is cheaper?

Repository:
  rC Clang

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

https://reviews.llvm.org/D57219