[PATCH] D123473: [LICM] Only create load in pre-header when promoting load.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D123473#3485355 <https://reviews.llvm.org/D123473#3485355>, @nlopes wrote:

> In D123473#3484702 <https://reviews.llvm.org/D123473#3484702>, @reames wrote:
>
>> I don't think we actually want to do that.  Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.
>
> You've a point. My concern is that it's surprising to have LLVM introducing a load in a function that's not supposed to do any load. I was thinking about I/O, but those accesses should be volatile. For kernel code it may not be ok, but probably they also use volatile to prevent the compiler messing around instead of relying on `writeonly`

Something can be surprising, and yet a natural consequence of other reasonable choices.  I think this is one of them.

On the topic of volatile, you raise an interesting point.  We don't seem to be suppressing speculation of volatile loads in the common utility.  That sounds like a bug, and likely worth a patch of it's own.  It's much broader than LICM though.  
.

> In D123473#3484702 <https://reviews.llvm.org/D123473#3484702>, @reames wrote:
>
>> If we accept that semantic, then this patch becomes completely unnecessary.
>
> Not true. The loop may not execute and thus you need the previous value. If the loaded value is poison, you still can't do the transformation,

Yes, you can.  That's my point.  If the code which branches on it executes, then you have UB.  Not before.

> Plus this patch removes useless loads; it's a perf win.

Hm, this a minor argument for folding load of caller accessible memory in writeonly to poison as an instcombine rule, but is pretty unconvincing here.

In D123473#3485641 <https://reviews.llvm.org/D123473#3485641>, @aqjune wrote:

> In D123473#3484702 <https://reviews.llvm.org/D123473#3484702>, @reames wrote:
>
>> I don't think we actually want to do that.  Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.
>
> If there is a transformation that turns load in a writeonly function into unreachable, defining it as UB would be necessary.
> However, my guess is that existing optimizations won't be that aggressive; perhaps for existing opts, poison might be enough. This is still my guess, though.

If you find it, let me know.  We can debate removing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123473