rjmccall added a comment.
> Didn't we work this out already when John added the alignment tracking stuff?
> I remember this bug involving libjpegturbo standalone assembly receiving a
> 32-bit argument, and then using the full 64-bit RDI register to read it, but
> clang stopped zero extending it.
So, this is complicated. LLVM IR's zext/sext representation is not explicit
about the width to which types are extended. Per the language reference, the
choice of extension width is at least target-specific, and I say "at least"
because nothing is obviously stopping it from also being type-specific; that
is, in theory `i1 zeroext` could mean an extension to 8-bit but `i8 zeroext`
could mean an extension to 32-bit or beyond. The use of
`isPromotableIntegerType()` here means that this only applies to integer types
of lower rank than `int`, which on x86_64 are all 16-bit or less; that
apparently embeds an assumption that the promotion width is 32-bit for this
target. libjpegturbo was incorrectly assuming that a formally 32-bit argument
was extended to the full 64-bit register width; obviously that's related to
what we're talking about here, but it's not actually affected by the proposed
patch.
Extending arguments to at least the width of `int` is necessary if you want
unprototyped functions with small arguments to be callable through a prototype.
The reverse isn't true, and no such concern applies to return types, so all in
all I doubt that this was an influence on Clang's behavior. We should already
be explicitly extending arguments to i32 when passing an unprototyped or
non-required argument.
> This change seems like it has backwards compatibility concerns with clang. If
> new clang stops extending things and it calls old clang code, won't that
> cause breakages? Based on the test code updates, it seems like you are
> updating both call sites and function prototypes. Won't that be problematic
> for the users that care about ABI stability over GCC ABI compatibility?
What defines the ABI for a particular target is also complicated. For
established targets with binary-compatibility requirements, ABI is arguably
best defined by what you're not willing to break compatibility with. Clang
supports several targets for which the system compiler is not GCC, and
therefore GCC's behavior is not particularly relevant to the discussion (for
those targets). I suspect that at least PS4 would strongly object to an ABI
change here, no matter what the ABI document says; I don't know about Darwin,
BSD, etc. So I think it is going to be necessary to make this
target-OS-specific.
For targets for which GCC clearly defines the ABI, though, I think we clearly
need to take this change.
================
Comment at: clang/test/CodeGenCXX/const-init-cxx11.cpp:536
void test() {
- // CHECK: call void @_ZN13InitFromConst7consumeIbEEvT_(i1 zeroext true)
+ // FIXME: This should probably be:
+ // call void @_ZN13InitFromConst7consumeIbEEvT_(i1 zeroext true)
----------------
rnk wrote:
> I agree it would be nice to avoid emitting these coercions through memory for
> such simple cases.
In the short term, yes, we should update the code so that we don't need a
coercion through memory to go from i1 to i8.
The right long-term change is that LLVM's zext/sext attributes should encode
the width to which the value is extended.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72742/new/
https://reviews.llvm.org/D72742
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
