[PATCH] D22840: Split the store of an int value merged from a pair of smaller values into multiple stores.

[Wei Mi via llvm-commits]

wmi added a comment.

Chandler, thanks for the review!

For the case I saw performance change, it only contains int-float pair. I agree int-float pair is a more clear win: for int-float pair, splitting wide store will save two bitwise instructions and one float-to-int conversion instruction but increase a store instruction, so it is two instructions saving.  For int-int pair, it is only one instruction saving so the saving is more blurred. Good suggestion, I will implement the target query.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12159-12162
@@ -12157,1 +12158,6 @@
 
+  if (TLI.isMultiStoresCheaperThanBitsMerge()) {
+    if (SDValue NewSt = splitMergedValStore(ST))
+      return NewSt;
+  }
+
----------------
chandlerc wrote:
> No need for braces around the outer if here, although it probably won't matter based on the refactoring I suggested...
Ok.  

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12173-12184
@@ +12172,14 @@
+/// Instruction sequence of i64 Store:
+///         t1: i32 = bitcast float_tmp
+///       t2: i64 = zero_extend t1
+///         t3: i64 = zero_extend i32_tmp
+///       t4: i64 = shl t3, Constant:i8<32>
+///     t5: i64 = or i64 t2, t4
+///   t6: ch = store<ST8[addr]> t0, t5, FrameIndex:i64<0>, undef:i64
+///
+/// Instruction sequence of splitted i32 stores:
+///     t1: i32 = bitcast float_tmp
+///   t2: ch = store<ST4[addr](align=8)> t0, t1, FrameIndex:i64<0>, undef:i64
+///     t3: i64 = add FrameIndex:i64<0>, Constant:i64<4>
+///   t4: ch = store<ST4[addr+4]> t0, i32_tmp, t3, undef:i64
+///
----------------
chandlerc wrote:
> You can write DAG examples using a more brief form that is pretty common:
> 
>   /// (store (or (zext (bitcast F to i32) to i64),
>   ///            (shl (zext I to i64), 32)), addr)
Ok. Will change it.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12217-12221
@@ +12216,7 @@
+  SDValue Shl, Lo, Hi;
+  if (Op1.getOpcode() == ISD::SHL) {
+    Shl = Op1;
+    Lo = Op2;
+    Hi = Shl.getOperand(0);
+  } else if (Op2.getOpcode() == ISD::SHL) {
+    Shl = Op2;
----------------
chandlerc wrote:
> A common slightly shorter idiom than this that we use is to test if Op1 is *not* SHL, and then swap, test again and bail, and then you know Op1 is the SHL and you can directly extract the Hi value.
That is definitely better. Will change it to swap. 

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12247
@@ +12246,3 @@
+    return SDValue();
+
+  // Start to split store.
----------------
chandlerc wrote:
> FWIW, here is where I would do the target query, passing in the inputs to the zero extends.
> 
> Then in the x86 implementation of the target query, I would look for a bitcast from a float, or whatever we end up with for the heuristic.
Thanks. I feel the target will end up here as you suggested. 


Repository:
  rL LLVM

https://reviews.llvm.org/D22840