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

Thu May 5 04:00:15 PDT 2022

fhahn added a comment.

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

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

While I think that's a great first data point, the coverage of writeonly functions is probably not too extensive unfortunately.  Another interesting data point may be to (randomly?) add `writeonly` attributes to the existing tests and verify that.

> I run LLVM's test suite under Alive2 and found no issues with the load-is-poison semantics for writeonly & argmemonly functions.
>
> Giving it a second thought, I agree with you. These attributes are not there to prevent optimizations, but to enable them. The compiler can do whatever it wants as long as it respects the memory model. The kernel may have a different idea about how the memory model looks like, but for now we don't support such special casing like gcc does (or used to).
>
> So I think we can change it. It's generally better to keep UB usage as low as possible, and yield poison whenever possible. So I favor the change.

I think it makes sense in isolation. But I think we should be careful to consider consistency with other/related attributes. At the moment, violations of a set of related memory attributes all have the same effect, UB. If we decide to change that, I think we should update at least all related attributes (`readnone`, `inaccessiblememonly`, `inaccessiblemem_or_argmemonly`, `argmemonly`, `!dereferenceable` metadata,...). For attributes that forbid writing to memory, we would need to retain UB?

Changing the langref is straight-forward, but enforcing it is difficult. Is there anything we could  do to be more confident when adjusting the semantics?

I added a test that triggers the assertion in the current version of the patch (3497a4f39601 <https://reviews.llvm.org/rG3497a4f396015c56057b6deb4cebe0caa07ab63a>).

> As for this patch, it optimizes the case where the loop & store are guaranteed to execute, so no load is needed. This is independent of writeonly.

Yeah, this should be an independent improvement. I am leaning towards landing the improvement without the assertion. And leave resolution of #51248 until we either adjust LangRef or add a bailout. I probably won't have time in the immediate future to work on updating the langref and possibly audit existing code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123473