This concern was raised in D98240. It's a miscompile and thanks for comments from @lebedev.ri.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Vectorize/VectorCombine.cpp | ||
---|---|---|
834 | This line is too cute for me, but ... |
Hm, this first needs more radical fixes - this is currently miscompiling vector indexes: https://alive2.llvm.org/ce/z/aWtH9w
I would suggest to first simply require the index to be constant.
Right, canScalarizeAccess() already does that, okay.
But then, it would be good to have a positive test for @insert_store_nonconst() :)
llvm/lib/Transforms/Vectorize/VectorCombine.cpp | ||
---|---|---|
835 | (alive *seems* to be happy with this) else NewAlignment = commonAlignment( NewAlignment, DL.getTypeStoreSize(NewElement->getType())); Please add positive tests: ; New alignment should be 8 define void @src(<8 x i64>* %q, i64 %s, i32 %idx) { %cmp = icmp ult i32 %idx, 2 call void @llvm.assume(i1 %cmp) %i = load <8 x i64>, <8 x i64>* %q, align 8 %vecins = insertelement <8 x i64> %i, i64 %s, i32 %idx store <8 x i64> %vecins, <8 x i64>* %q, align 8 ret void } ; New alignment should be 4 define void @src(<8 x i64>* %q, i64 %s, i32 %idx) { %cmp = icmp ult i32 %idx, 2 call void @llvm.assume(i1 %cmp) %i = load <8 x i64>, <8 x i64>* %q, align 4 %vecins = insertelement <8 x i64> %i, i64 %s, i32 %idx store <8 x i64> %vecins, <8 x i64>* %q, align 4 ret void } |
Please feel free to just directly commit new tests.
The new tests i asked for should be positive tests - they should be getting transformed (missing @llvm.assume())
llvm/test/Transforms/VectorCombine/load-insert-store.ll | ||
---|---|---|
128 | I think we still want those two tests i suggested, Please precommit the tests. |
llvm/test/Transforms/VectorCombine/load-insert-store.ll | ||
---|---|---|
23 | How do we justify this increase in alignment? |
llvm/test/Transforms/VectorCombine/load-insert-store.ll | ||
---|---|---|
23 | This change is correct. |
llvm/test/Transforms/VectorCombine/load-insert-store.ll | ||
---|---|---|
23 | 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 :). | |
128 | +1 - additional tests and pre-commit will make this easier to understand. |
llvm/test/Transforms/VectorCombine/load-insert-store.ll | ||
---|---|---|
128 | Let me know if I'm not seeing it, but we want 1 test with nonconst index where the original alignment is less than the presumed alignment for the new scalar store: define void @src(<8 x i64>* %q, i64 %s, i32 %idx) { %cmp = icmp ult i32 %idx, 2 call void @llvm.assume(i1 %cmp) %i = load <8 x i64>, <8 x i64>* %q, align 4 %vecins = insertelement <8 x i64> %i, i64 %s, i32 %idx store <8 x i64> %vecins, <8 x i64>* %q, align 2 ; make this different just to exercise the logic a bit more ret void } |
This line is too cute for me, but ...