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

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D156476#4540161 <https://reviews.llvm.org/D156476#4540161>, @nikic wrote:

> 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.

That's ok.
If we have

  store %foo, @glb
  call llvm.trap()

It's perfectly legal to remove the store as it's not observable. Trap doesn't read memory.

> 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 don't know why that equality must hold. A function that doesn't return doesn't have to write to memory. We can think of the state has having a `halt` bit, to where trap, exit(), etc, write to.  (exit() reads memory because it executes global destructors though, while trap doesn't).

That said, this change shouldn't even be needed because since trap doesn't return, whatever it writes to is not observable by the rest of the program. I think that `noreturn` can safely imply `write: none`.


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

https://reviews.llvm.org/D156476