[PATCH] D33675: [DAG] Do MergeConsecutiveStores again before Instruction Selection

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13170
+  // of MergeStores from happening. For now do the merging twice; before and
+  // after legalization.
+  if (!LegalTypes || (Level == AfterLegalizeDAG)) {
----------------
efriedma wrote:
> Can you point to an example testcase where the run before legalization matters?
The most obvious is:

merge_vec_element_store and merge_vec_extract_stores in CodeGen/X86/MergeConsecutiveStores  have issues related to bitcasting simplification. From what I've seen this is because we do a bticasting op for a vector with index 0 that we don't with others. 

Related issues that also need addressing before killing the pre-legalization merge. These seem simple enough:

* redundant_stores_merging and overlapping_stores_merging in CodeGen/X86/stores-merging.ll fails to merge because BaseIndexOffset cannot look through the X86ISD:Wrappers nor extract offset from TargetGlobals preventing merge otherwise obvious merges. The later is also true for CodeGen/AMDGPU/merge-stores.ll

* truncated stores are neither generated or used as valid inputs for store merge.

* CodeGen/AArch64/merge-store.ll  adds extra nodes due to bitcasting


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:9425
-    return SDValue();
 
   StoreSDNode *S = cast<StoreSDNode>(N);
----------------
efriedma wrote:
> How is this related to the other change?
Amongst other things split storess replaces vector stores of zero/scalars into appropriate sizes so that as Machine instructions we can create paired memory operations which may be cheaper. This should be run whenever we create a new store larger store. 

Concrete examples of the effects can be seen in the CodeGen/AArch64/ldst-opt.ll test. This patch nominally depends on D33518 which fixes untested cases caught by the recent store merge optimizations. 


https://reviews.llvm.org/D33675