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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
