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

Tue Nov 1 02:05:59 PDT 2022

nikic added a comment.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410