[PATCH] D104663: [OpaquePtr] Remove checking pointee type for byval/preallocated type

Mon Jun 21 17:46:32 PDT 2021

dblaikie added a comment.

In D104663#2831908 <https://reviews.llvm.org/D104663#2831908>, @rnk wrote:

> In D104663#2831661 <https://reviews.llvm.org/D104663#2831661>, @dblaikie wrote:
>
>> "arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?
>>
>> Is this an alternative to that patch that kept getting committed/reverted that I think was related to trying to unify call site and function declaration parameter attributes? Might be worth mentioning that one in this patch description for history/context?
>>
>> There are other attributes that don't seem to do this kind of "try the call instruction or the function declaration", do they? so I'd be a bit hesitant about this direction. Would like to get @rnk's thoughts on this too, perhaps.
>
> As someone who has all the context, the commit message makes sense to me. In the change you allude to, we tried to stand on the principle that a call instruction should carry all of the ABI attributes it needs, but in practice, we found that IR producers, in particular instrumentation passes, usually don't set ABI attributes on call sites. It should be a semantics-preserving transform to rinse the target of a call instruction through an identity function, but in practice, that would break all these IR producers. It would make most direct calls indirect, and the ABI attributes would no longer be correct.

Would we like to clean up those producers but it's infeasible - or is there good reason not to? (if it'd be good to do so, I'm OK with not necessarily doing it now - but writing it down as the intended goal/intentional technical debt for now)

Sounds like, if I'm following, that the "does everything work OK/the same if you rinse a target of a call through an identity function" test means we no longer think it's a good idea to try to check call attributes against their callee? Is it UB if they mismatch? (doesn't sound like it) are the callee's attributes ignored for the purpose of understanding call instructions? (that sounds correct/consistent with the rinse-test?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104663