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

Fri Dec 9 00:40:33 PST 2022

nikic added a comment.

In D134410#3898615 <https://reviews.llvm.org/D134410#3898615>, @nlopes wrote:

> In D134410#3898563 <https://reviews.llvm.org/D134410#3898563>, @nikic wrote:
>
>> 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.

I think the gain in expressiveness is that we can write something like `freeze(load)`. For example, D138766 <https://reviews.llvm.org/D138766> currently proposes to use a silly pattern like `bitcast(freeze(load(<4 x i8>)) to i32)` to achieve a "frozen load", because there is no other way to express it right now.

The other problem I see here is that we still need to do something about the memcpy -> load/store fold. As soon as we have poison from uninit values (either directly or via `!uninit_is_poison`) this will start causing miscompiles very quickly. The only way to do this right now is again with an `<N x i8>` vector load/store, which still optimizes terribly. This needs either load+store of integer to preserve poison, or again some form of byte type.

Something I've only recently realized is that we also run into this problem when inserting spurious load/store pairs, e.g. as done by LICM scalar promotion. If we're promoting say an i32 value to scalar that previously used a conditional store, then promotion will now introduce an unconditional load and store, which will spread poison from individual bytes. So that means that technically scalar promotion (and SimplifyCFG store speculation) also need to do some special dance to preserve poison. And without preservation of poison across load/store/phi in plain ints, this is going to be a bad optimization outcome either way: We'd have to use either a vector of i8 or a byte type for the inserted phi nodes and only cast to integer and back when manipulating the value, which would probably defeat the optimization. At that point probably best to freeze the first load (which again needs a freezing load).

(We should probably move the discussion around this patch series to discourse.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410