[PATCH] D74311: [CodeGen] Fix the computation of the alignment of split stores.

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for a comment nit and some potential test changes.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6873
+    if (Upper && SI.getAlign()) {
+      // When the original store is aligned, the lower part has the same alignment.
+      // Find the best alignment for the upper part. 
----------------
lebedev.ri wrote:
> On a first read, this is very confusing.
> When splitting store in half, naturally one half will retain the alignment
> of the original wider store, regardless of whether it was over-aligned or not,
> while other will require adjustment.
The suggested "one half" is better than the current "lower half" - eliminate the endian-specific inaccuracy.


================
Comment at: llvm/test/CodeGen/PowerPC/split-store-alignment-le.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -codegenprepare -force-split-store < %s  | FileCheck %s
+
----------------
Unless I'm missing some advantage of the way of the tests are arranged currently, I prefer that:
1. The tests live under llvm/test/Transforms/CodeGenPrepare/<target>. I know there are exceptions, but that is where I first look for IR --> IR tests.
2. If we include the layout specifiers as parameters in the RUN line, then it saves some duplication, and it's easier to spot the endian differences.
3. If we can push the non-crashing tests as a preliminary commit, that would be nicer, so we can see the current buggy code (and the tests will remain just in case this patch ever gets reverted).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74311