[PATCH] D103419: [VectorCombine] Fix alignment in single element store
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 8 09:45:43 PDT 2021
spatel added inline comments.
================
Comment at: llvm/test/Transforms/VectorCombine/load-insert-store.ll:23
; CHECK-NEXT: [[TMP0:%.*]] = getelementptr inbounds <8 x i16>, <8 x i16>* [[Q:%.*]], i32 0, i32 3
-; CHECK-NEXT: store i16 [[S:%.*]], i16* [[TMP0]], align 1
+; CHECK-NEXT: store i16 [[S:%.*]], i16* [[TMP0]], align 2
; CHECK-NEXT: ret void
----------------
lebedev.ri wrote:
> spatel wrote:
> > How do we justify this increase in alignment?
> > The original code had minimal `align 1`, so it could be anything. We are creating a scalar store at an address 6 bytes over that, so it could still be anything?
> This change is correct.
> Before `store <...>, align 1`, we have already established that the `%q` is more aligned,
> as per the `load <...>` with an implicit alignment, which isn't `1`.
> https://alive2.llvm.org/ce/z/C2qnUc
Ah, thanks for explaining. IIUC, we add explicit alignment to all load/store in IR now, so we should add the `align 16` to this test to avoid confusion - and a test comment would be nice too :).
================
Comment at: llvm/test/Transforms/VectorCombine/load-insert-store.ll:142
+define void @insert_store_nonconst_large_alignment(<4 x i32>* %q, i32 zeroext %s, i32 %idx) {
+; CHECK-LABEL: @insert_store_nonconst_large_alignment(
----------------
lebedev.ri wrote:
> I think we still want those two tests i suggested,
> they demonstrate that we don't increase alignment from the maximal one allowed.
>
> Please precommit the tests.
+1 - additional tests and pre-commit will make this easier to understand.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103419/new/
https://reviews.llvm.org/D103419
More information about the llvm-commits
mailing list