[PATCH] D102552: [GlobalOpt] recompute alignments for loads and stores of updated globals

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D102552#2761382 <https://reviews.llvm.org/D102552#2761382>, @spatel wrote:

> In D102552#2761376 <https://reviews.llvm.org/D102552#2761376>, @lebedev.ri wrote:
>
>> I think the bug is the other way around. For each scalar we are going to split off of aggregate,
>> we should first determine the alignment for scalar by determining what largest legal alignment
>> could it had as part of the aggregate (based on the alignment of the outer type, and offset).
>> Then, we shouldn't //need// to update uses, because by then we didn't change the alignment,
>> and if any use overestimated it, then it is, and was, UB.
>
> I think we are already correctly finding the alignment of the updated global itself (see inline code comment). But I'm not sure how that can propagate to potentially stale alignment specifiers on the users other than what I proposed here.
>
> The problem is that if we have an alignment that's known better than the minimum (and that alignment is correct, not UB, for the original code), it may not hold for the new inner type (see inline test comment).

That's precisely my point. Isn't it a bug that we reduce the alignment? Shouldn't we instead overalign the split-off scalar to the maximal non-UB alignment requested by the uses?

> Let me know if I'm not seeing it correctly (not too familiar with this!).




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

https://reviews.llvm.org/D102552