[llvm] r239539 - Fix merges of non-zero vector stores
Andrea Di Biagio
andrea.dibiagio at gmail.com
Thu Jun 11 12:17:57 PDT 2015
Hi Matt,
I think the problem is in method 'getMergedConstantVectorStore':
+ SmallVector<SDValue, 8> BuildVector;
+
+ for (const MemOpLink &Store : Stores)
+ BuildVector.push_back(cast<StoreSDNode>(Store.MemNode)->getValue());
+ return DAG.getNode(ISD::BUILD_VECTOR, SL, Ty, BuildVector);
+}
Your assumption is that the number of consecutive scalar stores is equal to
the number of elements in Ty.
However, that assumption is incorrect because Ty can have less elements
than the number of consecutive scalar stores in 'Stores'.
Method 'MergeStoresOfConstantsOrVecElts' defines Ty as:
EVT Ty = EVT::getVectorVT(*DAG.getContext(), MemVT, NumElem);
Where: NumElem may be smaller than the number of elements in 'StoreNodes'.
I was able to reproduce the assertion failure with:
;
define void @foo(i32* f) {
entry:
store i32 0, i32* %f, align 4
%idx1 = getelementptr inbounds i32, i32* %f, i64 1
store i32 0, i32* %idx1, align 4
%idx2 = getelementptr inbounds i32, i32* %f, i64 2
store i32 0, i32* %idx2, align 4
%idx3 = getelementptr inbounds i32, i32* %f, i64 3
store i32 0, i32* %idx3, align 4
%idx4 = getelementptr inbounds i32, i32* %f, i64 4
store i32 0, i32* %idx4, align 4
ret void
}
;
$ llc -mtriple=x86_64-unknown-unknown -mcpu=generic
Here, the number of consecutive stores is 5. However, the number of
elements in vector type 'Ty' is 4.
I hope this helps.
-Andrea
On Thu, Jun 11, 2015 at 6:30 PM, Matt Arsenault <arsenm2 at gmail.com> wrote:
>
> On Jun 11, 2015, at 10:20 AM, Reid Kleckner <rnk at google.com> wrote:
>
> We're getting this assertion in the blame range 239540 - 239539 inclusive,
> so I'm pretty sure it's this change:
> SelectionDAG.cpp:756: void VerifySDNode(llvm::SDNode*): Assertion
> `N->getNumOperands() == N->getValueType(0).getVectorNumElements() && "Wrong
> number of operands!"' failed.
>
> This is while building freetype as part of chromium:
>
> http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/1809/steps/compile/logs/stdio
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__build.chromium.org_p_chromium.fyi_builders_ClangToTLinux_builds_1809_steps_compile_logs_stdio&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hDqvsZVl0aZXSFF_FX3q9YBwaW-yo4Gf9eI-F_jbk_4&s=GdvW8rJcyRxypctibGV10ihuVbk9MXFsMm3EnyCxvoE&e=>
>
> Do you think you can spot the problem, or do you want me to try to reduce
> a test case?
>
>
> A testcase would be useful
>
>
>
> On Thu, Jun 11, 2015 at 9:03 AM, Matt Arsenault <Matthew.Arsenault at amd.com
> > wrote:
>
>> Author: arsenm
>> Date: Thu Jun 11 11:03:52 2015
>> New Revision: 239539
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=239539&view=rev
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D239539-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hDqvsZVl0aZXSFF_FX3q9YBwaW-yo4Gf9eI-F_jbk_4&s=E3ewhgq2gCitd4Pmi-cJ95DiyqppJiQ-KZt3FZit12E&e=>
>> Log:
>> Fix merges of non-zero vector stores
>>
>> Now actually stores the non-zero constant instead of 0.
>> I somehow forgot to include this part of r238108.
>>
>> The test change was just an independent instruction order swap,
>> so just add another check line to satisfy CHECK-NEXT.
>>
>> Modified:
>> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> llvm/trunk/test/CodeGen/R600/merge-stores.ll
>> llvm/trunk/test/CodeGen/X86/2012-11-28-merge-store-alias.ll
>>
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=239539&r1=239538&r2=239539&view=diff
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_CodeGen_SelectionDAG_DAGCombiner.cpp-3Frev-3D239539-26r1-3D239538-26r2-3D239539-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hDqvsZVl0aZXSFF_FX3q9YBwaW-yo4Gf9eI-F_jbk_4&s=thO1pNNeDMmjVyxgU23FPueh_-jiS4cg693QDBpzTFs&e=>
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Thu Jun 11
>> 11:03:52 2015
>> @@ -387,6 +387,13 @@ namespace {
>> unsigned SequenceNum;
>> };
>>
>> + /// This is a helper function for MergeStoresOfConstantsOrVecElts.
>> Returns a
>> + /// constant build_vector of the stored constant values in Stores.
>> + SDValue getMergedConstantVectorStore(SelectionDAG &DAG,
>> + SDLoc SL,
>> + ArrayRef<MemOpLink> Stores,
>> + EVT Ty) const;
>> +
>> /// This is a helper function for MergeConsecutiveStores. When the
>> source
>> /// elements of the consecutive stores are all constants or all
>> extracted
>> /// vector elements, try to merge them into one larger store.
>> @@ -10576,6 +10583,17 @@ struct BaseIndexOffset {
>> };
>> } // namespace
>>
>> +SDValue DAGCombiner::getMergedConstantVectorStore(SelectionDAG &DAG,
>> + SDLoc SL,
>> + ArrayRef<MemOpLink>
>> Stores,
>> + EVT Ty) const {
>> + SmallVector<SDValue, 8> BuildVector;
>> +
>> + for (const MemOpLink &Store : Stores)
>> + BuildVector.push_back(cast<StoreSDNode>(Store.MemNode)->getValue());
>> + return DAG.getNode(ISD::BUILD_VECTOR, SL, Ty, BuildVector);
>> +}
>> +
>> bool DAGCombiner::MergeStoresOfConstantsOrVecElts(
>> SmallVectorImpl<MemOpLink> &StoreNodes, EVT MemVT,
>> unsigned NumElem, bool IsConstantSrc, bool UseVector) {
>> @@ -10606,12 +10624,7 @@ bool DAGCombiner::MergeStoresOfConstants
>> EVT Ty = EVT::getVectorVT(*DAG.getContext(), MemVT, NumElem);
>> assert(TLI.isTypeLegal(Ty) && "Illegal vector store");
>> if (IsConstantSrc) {
>> - // A vector store with a constant source implies that the constant
>> is
>> - // zero; we only handle merging stores of constant zeros because
>> the zero
>> - // can be materialized without a load.
>> - // It may be beneficial to loosen this restriction to allow
>> non-zero
>> - // store merging.
>> - StoredVal = DAG.getConstant(0, DL, Ty);
>> + StoredVal = getMergedConstantVectorStore(DAG, DL, StoreNodes, Ty);
>> } else {
>> SmallVector<SDValue, 8> Ops;
>> for (unsigned i = 0; i < NumElem ; ++i) {
>>
>> Modified: llvm/trunk/test/CodeGen/R600/merge-stores.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/R600/merge-stores.ll?rev=239539&r1=239538&r2=239539&view=diff
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_R600_merge-2Dstores.ll-3Frev-3D239539-26r1-3D239538-26r2-3D239539-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hDqvsZVl0aZXSFF_FX3q9YBwaW-yo4Gf9eI-F_jbk_4&s=mg6oFnnkyCij6op6RWDVntjpeZTF2GIXWsMkyMtlo4E&e=>
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/R600/merge-stores.ll (original)
>> +++ llvm/trunk/test/CodeGen/R600/merge-stores.ll Thu Jun 11 11:03:52 2015
>> @@ -89,7 +89,11 @@ define void @merge_global_store_2_consta
>> }
>>
>> ; GCN-LABEL: {{^}}merge_global_store_2_constants_f32_i32:
>> -; GCN: buffer_store_dwordx2
>> +; SI-DAG: s_mov_b32 [[SLO:s[0-9]+]], 4.0
>> +; SI-DAG: s_movk_i32 [[SHI:s[0-9]+]], 0x7b{{$}}
>> +; SI-DAG: v_mov_b32_e32 v[[VLO:[0-9]+]], [[SLO]]
>> +; SI-DAG: v_mov_b32_e32 v[[VHI:[0-9]+]], [[SHI]]
>> +; GCN: buffer_store_dwordx2 v{{\[}}[[VLO]]:[[VHI]]{{\]}}
>> define void @merge_global_store_2_constants_f32_i32(float addrspace(1)*
>> %out) #0 {
>> %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
>> %out.gep.1.bc = bitcast float addrspace(1)* %out.gep.1 to i32
>> addrspace(1)*
>> @@ -99,7 +103,11 @@ define void @merge_global_store_2_consta
>> }
>>
>> ; GCN-LABEL: {{^}}merge_global_store_4_constants_i32:
>> -; GCN: buffer_store_dwordx4
>> +; GCN-DAG: v_mov_b32_e32 v[[HI:[0-9]+]], 0x14d{{$}}
>> +; GCN-DAG: v_mov_b32_e32 v{{[0-9]+}}, 0x1c8{{$}}
>> +; GCN-DAG: v_mov_b32_e32 v{{[0-9]+}}, 0x7b{{$}}
>> +; GCN-DAG: v_mov_b32_e32 v[[LO:[0-9]+]], 0x4d2{{$}}
>> +; GCN: buffer_store_dwordx4 v{{\[}}[[LO]]:[[HI]]{{\]}}
>> define void @merge_global_store_4_constants_i32(i32 addrspace(1)* %out)
>> #0 {
>> %out.gep.1 = getelementptr i32, i32 addrspace(1)* %out, i32 1
>> %out.gep.2 = getelementptr i32, i32 addrspace(1)* %out, i32 2
>>
>> Modified: llvm/trunk/test/CodeGen/X86/2012-11-28-merge-store-alias.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/2012-11-28-merge-store-alias.ll?rev=239539&r1=239538&r2=239539&view=diff
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_X86_2012-2D11-2D28-2Dmerge-2Dstore-2Dalias.ll-3Frev-3D239539-26r1-3D239538-26r2-3D239539-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hDqvsZVl0aZXSFF_FX3q9YBwaW-yo4Gf9eI-F_jbk_4&s=C17Vcb6j3xcCy9zvTgW8MGIvo7fIAfOpo3Y3Ccnnrbk&e=>
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/X86/2012-11-28-merge-store-alias.ll (original)
>> +++ llvm/trunk/test/CodeGen/X86/2012-11-28-merge-store-alias.ll Thu Jun
>> 11 11:03:52 2015
>> @@ -3,6 +3,7 @@
>> ; CHECK: merge_stores_can
>> ; CHECK: callq foo
>> ; CHECK: xorps %xmm0, %xmm0
>> +; CHECK-NEXT: movl 36(%rsp), %ebp
>> ; CHECK-NEXT: movups %xmm0
>> ; CHECK: callq foo
>> ; CHECK: ret
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150611/0366c277/attachment.html>
More information about the llvm-commits
mailing list