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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
