[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2)

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D134410#3898563 <https://reviews.llvm.org/D134410#3898563>, @nikic wrote:

> In D134410#3895253 <https://reviews.llvm.org/D134410#3895253>, @nlopes wrote:
>
>> In D134410#3895077 <https://reviews.llvm.org/D134410#3895077>, @nikic wrote:
>>
>>> In D134410#3894995 <https://reviews.llvm.org/D134410#3894995>, @nlopes wrote:
>>>
>>>> We wanted this patch to make us switch uninitialized loads to poison at will, since they become UB. In practice, this helps us fixing bugs in SROA and etc without perf degradation.
>>>
>>> Can you elaborate on this? I don't see how this is necessary for switching uninitialized loads to poison.
>>
>> It's not mandatory, it's a simple way of achieving it as !noundef already exists.
>>
>> We cannot change the default behavior of load as it would break BC.
>
> FWIW, I don't agree with this. It's fine to change load semantics, as long as bitcode autoupgrade is handled correctly. I'd go even further and say that at least long term, any solution that does not have a plain `load` instruction return `poison` for uninitialized memory will be a maintenance mess.
>
> I think the core problem that prevents us from moving in this direction is that we have no way to represent a frozen load at all (as `freeze(load)` will propagate poison before freezing). If I understand correctly, you're trying to solve this by making *all* loads frozen loads, and use `load !noundef` to allow dropping the freeze. I think this would be a very bad outcome, especially as we're going to lose that `!noundef` on most load speculations, and I don't think we want to be introducing freezes when speculating loads (e.g. during LICM).
>
> I expect that the path of least resistance is going to be to support bitwise poison for load/store/phi/select and promote it to full-value poison on any other operation, allowing a freezing load to be expressed as `freeze(load)`.
>
> Please let me know if I completely misunderstood the scheme you have in mind...

You got it right. But the load we propose is not exactly a freezing load. It only returns `freeze poison` for uninit memory. It doesn't freeze stored values.
If we introduce a `!uninit_is_poison` flag, we can drop !noundef when hoisting and add `!uninit_is_poison` instead (it's implied by !noundef).

The question is what's the default for uninit memory: `freeze poison` or `poison`? But I think we need both. And since we need both (unless we add a freezing load or bitwise poison), why not keep a behavior closer to the current?
A freezing load is worse as a store+load needs to be forwarded though a freeze, as the load is not a NOP anymore.

Bitwise poison would be nice, but I don't know how to make it work. To make it work with bit-fields, we would need and/or to propagate poison bit-wise as well. But then we will break optimizations that transform between those and arithmetic. Then you start wanting that add/mul/etc also propagate poison bit-wise and then I don't know how to specify that semantics.  (if you want, I can forward you a proposal we wrote a few years ago, but we never managed to make sound, so it was never presented in public)

I agree that bit-wise poison for loads sounds appealing (for bit fields, load widening, etc), but without support in the rest of the IR, it's not worth much. If it becomes value-wise in the 1st operation, then I don't think we gain any expressivity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410