[PATCH] D156476: [IR] Mark `llvm.trap` as `memory(inaccessiblemem: write)`

Thu Jul 27 12:48:15 PDT 2023

nikic added a comment.

In D156476#4540118 <https://reviews.llvm.org/D156476#4540118>, @jdoerfert wrote:

> In D156476#4540070 <https://reviews.llvm.org/D156476#4540070>, @nikic wrote:
>
>> Making it inaccessible mem only is certainly correct -- but I'm wondering: Does it even need to have a memory effect at all? After all, it already has a divergence effect by dint of not being willreturn, so it's not legal to drop llvm.trap anyway.
>
> So with inaccessiblemem and with read none we can loose updates on state prior to the trap, but I think those are outside of our abstract machine.
> The only problem I see is a must progress function containing a trap. We could eliminate the call, couldn't we?

Yeah, good point. mustprogress + readonly = willreturn, so we need to force a write memory effect to avoid that. In that case `memory(inaccessiblemem: write)` does seem like the best we can do.

> I think because we also have assume, the best way forward is `memory(artificial: write)` for both, or, if we want to have an easier time dropping transitive assumes:
> `memory(terminate: write)` for trap, and `memory(assume: write)` for llvm.assume. Then you can drop calls to assume only functions if you would not inline them, etc.

FWIW, I don't think I've ever encountered an optimization failure that was caused by the inaccessiblemem effect on assume (well, apart from the fact that it has any at all -- code that only checks mayWriteToMemory without going though AA does cause problems sometimes). Not opposed to making it more precise, but I'm not sure whether the added precision will really bring much practical value.

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

https://reviews.llvm.org/D156476