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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444
   case BO_NE:
+    return Builder.CreateICmpNE(FullLHS, FullRHS);
+  case BO_Mul:
----------------
ebevhan wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > 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?
> > > > > > Is this written down somewhere? Are there targets that use the opposite ABI rule that we might need to support someday?
> > > > > 
> > > > > What exactly do you mean by 'opposite'? That the padding is cleared after every operation?
> > > > > 
> > > > > It was (and still is) the case in our downstream implementation that all unsigned operations explicitly clear the padding bit after every operation, leading to defined behavior on overflow for such types.
> > > > 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.
> > > Okay.  If we ever have a target that uses an ABI that (1) includes padding but (2) considers it to be "unspecified" in the sense of "it can validly be non-zero", we'll have to change this code, but I agree it's the right move to not preemptively generalize to cover that possibility.
> > To clarify on our use case, we have the padding on the unsigned types mostly to harmonize the behavior of unsigned and signed types. The only real native support is for signed types, but if the unsigned types are padded and then use signed operations to implement them, you get (almost) the same level of performance out of unsigned ones.
> > 
> > The primary difference between the upstream CG and our implementation is that the padding bit is always set to zero after operations. This means that overflow on the unsigned types is essentially defined as modulo wraparound instead of UB, and unsigned types will always contain a valid value.
> Oops, did not mean to submit that comment further up. Was left over from an earlier session.
That's still effectively the same ABI, just (as you say) intentionally defining the result of non-saturating overflow.   I assume you haven't completely eliminated UB and that you still consider an unsigned value with 1 in the padding bit (which you never produce from arithmetic, but which could be produced by e.g. memcpy) to be an illegal representation.  What I mean by "opposite" would be an ABI in which that was *not* an illegal representation, and which instead defined the padding bit to be ignored.  That ABI would require IRGen to clear the bit before making these comparisons.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57219