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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444
   case BO_NE:
+    return Builder.CreateICmpNE(FullLHS, FullRHS);
+  case BO_Mul:
----------------
rjmccall wrote:
> 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?
Yes. We assume inputs are zero padded, and that non-saturating overflow is undefined so we do not need to clear the padding after writing a new value. Sorry for the misunderstanding. I see what you mean by guaranteed zero.

Overflow in general is controlled by the `FX_FRACT_OVERFLOW` and `FX_ACCUM_OVERFLOW` pragmas, where if these are set to `DEFAULT`, operations that can overflow with these types is undefined according to the standard (4.1.3). In terms of padding bits, clause 6.2.6.3 mentions that the values of padding bits are "unspecified". I imagine this means that we can assume the value to be whatever we want it to, so we can assume that these values are a guaranteed zero.

I believe @ebevhan requested this being added since his downstream implementation used padding to match the scales of signed and unsigned types, so he may be able to offer more information on targets with different ABIs. We don't plan to use any special hardware, so we're following the "typical desktop processor" layout that uses the whole underlying int and no padding (mentioned in Annex 3).

In the same section, the standard also mentions types for other targets that may have padding, so there could be some value in indicating to the backend that for these particular targets, this part of the operand is padding, so select an appropriate operation that performs a comparison on this size type. But I don't know much about these processors and would just be guessing at how they work.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57219