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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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)) {
----------------
niravd wrote:
> 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
> truncated stores are neither generated or used as valid inputs for store merge.

Oh, so it completely breaks everything that isn't x86. :)

Could you make before/after/both an option, so it's easier to check the impact if we do run into issues?

If we really just want to run this after legalization, it's probably a good idea to flip the switch as soon as possible, even if it causes minor regressions.


https://reviews.llvm.org/D33675