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

Mon May 9 08:01:22 PDT 2022

nlopes added a comment.

In D123473#3493474 <https://reviews.llvm.org/D123473#3493474>, @fhahn wrote:

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

Agreed. There are very few tests for writeonly. The good news is that it isn't heavily used either.

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

This kind of attributes are usually added by LLVM itself, they are not specified by hand by users. Hence we are more free to select the semantics.
There are valid reasons to stop the compiler from hoisting loads, but I don't think writeonly is the right attribute. As it was probably added by LLVM to denote a function that is functional, may write to memory, but it will always write the same thing. So repeated calls can be removed. Introducing loads that yield poison doesn't change a thing, as long as their result is not used at run-time.
So for the uses of writeonly that we have today, I think yield poison is sufficient. Nevertheless, (part of) your patch is still useful regardless of the semantics to avoid creating useless loads.

> For attributes that forbid writing to memory, we would need to retain UB?

Yes, as we use the "nowrite" attributes to determine whether a function can be hoisted and etc, so we can't introduce a store. Also, introducing stores is already forbidden in many cases.

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

The best thing we have today is fuzzing + Alive2. @regehr has been working on the fuzzing side of things. This strategy has been reasonably successful in testing semantics. The only caveat is that Alive2 doesn't support IPO, so we disable it for the Attributor and related things to avoid false-positives. For attributes that's not ideal.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123473