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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

The approach here looks awesome.

Mostly really minor nits about comments, and some suggestions to minimize and clean up the test case.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12208
@@ +12207,3 @@
+/// are bundled together as an i64 value before being stored into memory.
+/// Quite often it is more efficent to generate separate stores for F and I,
+/// which can remove the bitwise instructions or sink them to colder places.
----------------
I would just say "Sometimes" here to avoid people defaulting a new target into splitting the stores without measuring.

================
Comment at: lib/Target/X86/X86ISelLowering.h:769-771
@@ +768,5 @@
+                                           SDValue Hi) const override {
+      // If the pair to store is a mixture of float and int values, we will
+      // save two bitwise instructions and one float-to-int instruction and
+      // increase one store instruction. It is more likely a win.
+      if (Lo.getOpcode() == ISD::BITCAST || Hi.getOpcode() == ISD::BITCAST) {
----------------
It's probably also worth noting that beyond the instruction count difference, there is potentially a more significant benefit because it avoids the float->int domain switch for input value. I have a suspicion that is a significant part of the savings here.

================
Comment at: lib/Target/X86/X86ISelLowering.h:778-780
@@ +777,5 @@
+      }
+      // If the pair only contains int values, we will save two bitwise
+      // instructions and increase one store instruction. We leave the case
+      // out for now and wait until we find a case showing it is beneficial.
+      return false;
----------------
And here it is probably worth mentioning the other upside of only doing a single memory operation in addition to having minimal instruction count overhead.

================
Comment at: test/Transforms/InstCombine/split-store.ll:16-31
@@ +15,18 @@
+; CHECK: leaq (%rsp), %rdi
+define void @int32_float_pair(i32 %tmp1, float %tmp2) local_unnamed_addr {
+entry:
+  %ref.tmp = alloca i64, align 8
+  %tmpcast = bitcast i64* %ref.tmp to %"pair1"*
+  %t0 = bitcast i64* %ref.tmp to i8*
+  call void @llvm.lifetime.start(i64 8, i8* %t0)
+  %t1 = bitcast float %tmp2 to i32
+  %retval.sroa.2.0.insert.ext.i = zext i32 %t1 to i64
+  %retval.sroa.2.0.insert.shift.i = shl nuw i64 %retval.sroa.2.0.insert.ext.i, 32
+  %retval.sroa.0.0.insert.ext.i = zext i32 %tmp1 to i64
+  %retval.sroa.0.0.insert.insert.i = or i64 %retval.sroa.2.0.insert.shift.i, %retval.sroa.0.0.insert.ext.i
+  store i64 %retval.sroa.0.0.insert.insert.i, i64* %ref.tmp, align 8
+  call void @goo1(%"pair1"* dereferenceable(8) %tmpcast)
+  call void @llvm.lifetime.end(i64 8, i8* %t0)
+  ret void
+}
+
----------------
It would be good to minimize this test case rather than keeping it so close to what happens to come out of Clang for a particular C++ input.

I'd just write direct tests in LLVM IR. Something like:

  define void @test1(i32 %i, float %f, i64* %ptr) {
  entry:
    ... = zext ...
    ... = bitcast ...
    ... = zext ...
    ... = shl ...
    ... = or ...
    store ..., i64* %ptr
    ret void
  }




Repository:
  rL LLVM

https://reviews.llvm.org/D22840