ebevhan added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444 case BO_NE: + return Builder.CreateICmpNE(FullLHS, FullRHS); + case BO_Mul: ---------------- 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. 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
