[PATCH] D135738: [InstCombine] Bail out of casting calls when a conversion to byval is involved.

Wed Oct 12 12:01:12 PDT 2022

nikic added a comment.

In D135738#3853262 <https://reviews.llvm.org/D135738#3853262>, @efriedma wrote:

> IR optimizations accommodate mismatches to some extent; if the types of the caller and the callee don't match, we treat the call as opaque, which effectively makes the call work if the actual registers match.  But that doesn't detect ABI-modifying attributes.
>
> It might be possible to adjust optimizations to handle this conservatively without any deep infrastructure changes.  Basically, treat calls as opaque if there are significant ABI attributes that don't match... but I'm not sure what exactly that check would include, or how we make it fast.
>
> ----
>
> That said, I don't have any objection to this patch... I mean, it can't really do any harm.  I just want to be sure we're on the same page about what it does, and does not accomplish.

My problem with this patch is that it does not address the problem in a principled way: It works around one specific instance of this problem, while retaining it in many other places. As you already pointed out, inlining for the struct containing a pointer case is broken. From a quick try, the same is true for argument promotion (https://llvm.godbolt.org/z/8We7dvYvT), we mix up the levels of indirection. The same is probably true for other IPO transforms as well. There's probably also issues with attribute interpretation that are much more wide-spread (e.g. does a `nonnull` refer to the byval pointer or its payload?)

If we wanted to support this, I think we would have to make an effort to support it more broadly. And I personally don't think this is justified, especially as this will need to be fixed on the rustc side anyway (if nothing else then to enable cross-language inlining after these changes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135738