[PATCH] D155900: [TTI][NFCI] Introduce two new target transform hooks

Fri Jul 21 00:34:02 PDT 2023

nikic requested changes to this revision.
nikic added a subscriber: aeubanks.
nikic added a comment.
This revision now requires changes to proceed.

Can you please explain what the motivation for this is? I mean, I can see the benefit in theory, but I'm not clear on where this would help in practice.

This seems like a very dangerous change to me. If we want to do this, I'd recommend to go for a default implementation of "always valid" and then carve out very specific exceptions based on real-world motivation.

Two immediate issues I see here are:

- On x86_32 at least, clang will pass arguments by value, even though they really have indirect byval ABI. The reasoning is that LLVM will actually lowering those arguments with the correct ABI, and passing values as SSA values is of course much more amenable to optimization. However, Rust will instead generate the "correct" byval ABI. These calls are incompatible at the IR level (you will have a byval ptr argument on one side an something like i32 on the other -- or worse, a non-byval ptr that has a completely different meaning!) but become compatible after lowering. Clang is doing something extremely fishy here, but we still need to make sure that this does not get miscompiled (as your current code will do, on multiple levels).
- There are recurring problems with callee vs caller ABI attribute mismatches. IIRC there were some attempts to clean this up as part of the opaque pointer migration, by making lowering only consider callee ABI attributes. However, this ran into a whole series of issues with attribute mismatches in practice, and ended up getting reverted. Maybe @aeubanks has more context on that. Missing signext/zeroext attributes are also a recurring problem for targets like s390x. And this also plays into the previous point: A ptr with and without byval are not necessarily an ABI mismatch after lowering.

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

https://reviews.llvm.org/D155900