[PATCH] D136284: SROA should freeze undefs for loads with no prior stores

Fri Oct 21 08:58:58 PDT 2022

nlopes added a comment.

In D136284#3874755 <https://reviews.llvm.org/D136284#3874755>, @jamieschmeiser wrote:

> In D136284#3874614 <https://reviews.llvm.org/D136284#3874614>, @nlopes wrote:
>
>> In D136284#3874596 <https://reviews.llvm.org/D136284#3874596>, @jamieschmeiser wrote:
>>
>>> In D136284#3874492 <https://reviews.llvm.org/D136284#3874492>, @nikic wrote:
>>>
>>>> At least in C++, working with uninitialized memory is pretty much always immediate undefined behavior, see https://eel.is/c++draft/basic.indet for the relevant wording. The only exception are "copy-like" operations on unsigned character types, which comparisons do not fall under.
>>>>
>>>> I believe the C specification is less clear cut about this, but Clang and LLVM assume basically the same to also hold for C code.
>>>
>>> What version of the C++ standard is this?  Every version that I have seen has basics as section 3 and I cannot find this section, nor anything similar.  Section 6 is Statements.
>>
>> That discussion is orthogonal to this patch.
>> This patch is not desired because it's not needed per the current LLVM IR semantics. If you want to change something, you need to start by proposing a change to the LLVM IR semantics. You'll need to justify why it's needed, why it's correct, the perf impact, how to make it backwards compatible, why it's better than the proposals over the table right now.
>>
>> Anyway, a patch like this solves no problem. LLVM allows loads to be duplicated. Your patch does nothing to prevent that and to ensure all loads see the same value. The issue is way more complicated than what this patch implies.
>
> I'm not trying to flog a dead horse (I've already abandoned this) but I am trying to understand this statement.  I do not dispute that there may be other situations similar to this but, assuming that we did want to ensure that the loads had the same value, why is freezing them at this point not the correct thing to do?  Whether they are poison or undef, freezing them would ensure that they compare equal.  Yes, I understand it may have performance impacts, there may be better ways, etc.  But, ignoring all that, isn't this exactly what freeze is designed for?

It's not enough to ensure the semantics you want. What about optimizations that happen before and after SROA? This patch only deals with a subset of the cases (the ones that are detected by SROA's algorithm). Again, load duplication happens, and this patch doesn't deal with it. So it's inconsistent.
To implement the proposed semantics, you would need to change quite a few optimizations, not just SROA.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136284