[PATCH] D103419: [VectorCombine] Fix alignment in single element store

[Sanjay Patel via Phabricator via llvm-commits]

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