[PATCH] D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI.

[John McCall via Phabricator via cfe-commits]

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