[X86] Break false dependencies before partial register updates when the source operand is in memory

[mkuper]

Summary

The list of opcodes false-dependency-breaking looks at consists only of register-source variants, but not memory-source.
It doesn't look like there's a good reason for that, since memory-source variants suffer from the same problem - e.g. the loop in the attached testcase gets a roughly 3x performance improvement on sandybridge.

[RKSimon]

Thanks for looking at this Michael.

Why didn't you add the ROUNDS*, RSQRTSS and SQRTS* m_Int variants?

Also I believe the SQRTSD ops can be added as well?

[mkuper]

Thanks a lot for the review, Simon.

ROUNDS* don't currently have m_Int variants defined.
I thought RSQRTSS and SQRTSS don't have them defined either, but apparently they do and I just missed them, I'll add them, thanks!

Regarding SQRTSD, I don't see why the r variants weren't on the original list. I'll add all variants.

[mkuper]

Heh.

Apparently even though SQRTSD was never on the list, there was a test that checked that memory operands into SQRTSD were not load-folded. The test apparently passed not because of this code, but because they weren't folded regardless. At some point, Manman improved the folding logic, and changed the test to verify that SQRTSD memory operands ARE in fact folded.

Rolling the test back as well.

[RKSimon]

Please can you double check that cvtsd2ss test and if possible add tests for what other partial ops you can?

Also - should your test case be under sse-domains.ll or one of the break-*-dep.ll test files?

Unless you are wanting your reviewers to confirm I'm happy to approve this after that.

Cheers, Simon.

test/CodeGen/X86/break-sse-dep.ll

18 ↗

(On Diff #17261)

Should cvtsd2ss be folded?

[mkuper]

Regarding break-sse-dep vs. sse-domains, there are actually 3 different things going on:

1. Avoiding load-folding that creates false dependencies.
2. Avoiding domain crossing.
3. Breaking false dependencies that are already present in the code.

Before this patch, break-sse-dep.ll checked (1), and sse-domains.ll checked (2) and (3), and I've added another test checking (3) into sse-domains.
Do you think it's worth moving (3) together with (1) instead?

test/CodeGen/X86/break-sse-dep.ll

18 ↗

(On Diff #17261)

Yes, the optsize versions should be folded.
(The test is equivalent to t1 except for the optsize attribute).

[RKSimon]

In D6620#101342, @mkuper wrote:

Regarding break-sse-dep vs. sse-domains, there are actually 3 different things going on:

1. Avoiding load-folding that creates false dependencies.
2. Avoiding domain crossing.
3. Breaking false dependencies that are already present in the code.

Before this patch, break-sse-dep.ll checked (1), and sse-domains.ll checked (2) and (3), and I've added another test checking (3) into sse-domains.
Do you think it's worth moving (3) together with (1) instead?

It appears tests for (1) and (3) are the main contributions from your testcase so I think it'd be more relevant to being included in break-sse-dep.ll

test/CodeGen/X86/break-sse-dep.ll

18 ↗

(On Diff #17261)

Ah yes - missed that bit (just thought it was the inverse of t1).

[atrick]

LGTM

lib/Target/X86/X86InstrInfo.cpp

 /// Instead of
 ///   cvtss2sd (%rdi), %xmm0
 ///
 /// FIXME: This should be turned into a TSFlags.
 ///
 static bool hasPartialRegUpdate(unsigned Opcode) {
   switch (Opcode) {
   case X86::CVTSI2SSrr:
+  case X86::CVTSI2SSrm:
   case X86::CVTSI2SS64rr:
+  case X86::CVTSI2SS64rm:
   case X86::CVTSI2SDrr:
+  case X86::CVTSI2SDrm:
   case X86::CVTSI2SD64rr:
+  case X86::CVTSI2SD64rm:
   case X86::CVTSD2SSrr:
+  case X86::CVTSD2SSrm:
   case X86::Int_CVTSD2SSrr:
+  case X86::Int_CVTSD2SSrm:
   case X86::CVTSS2SDrr:
+  case X86::CVTSS2SDrm:
   case X86::Int_CVTSS2SDrr:
+  case X86::Int_CVTSS2SDrm:
   case X86::RCPSSr:
+  case X86::RCPSSm:
   case X86::RCPSSr_Int:
+  case X86::RCPSSm_Int:
   case X86::ROUNDSDr:
+  case X86::ROUNDSDm:
   case X86::ROUNDSDr_Int:
   case X86::ROUNDSSr:
+  case X86::ROUNDSSm:
   case X86::ROUNDSSr_Int:
   case X86::RSQRTSSr:
+  case X86::RSQRTSSm:
   case X86::RSQRTSSr_Int:
   case X86::SQRTSSr:
+  case X86::SQRTSSm:
   case X86::SQRTSSr_Int:
     return true;
   }
 
   return false;
 }
 
 /// getPartialRegUpdateClearance - Inform the ExeDepsFix pass how many idle

test/CodeGen/X86/sse-domains.ll

for.body:                                         ; preds = %entry, %for.body
   br i1 %tobool, label %for.end, label %for.body
 
 for.end:                                          ; preds = %for.body, %entry
   %s1.0.lcssa = phi float [ 0.000000e+00, %entry ], [ %add, %for.body ]
   %s2.0.lcssa = phi float [ 0.000000e+00, %entry ], [ %add2, %for.body ]
   %sub = fsub float %s1.0.lcssa, %s2.0.lcssa
   ret float %sub
 }
+
+; This loop contains a cvtsi2sd instruction that has a loop-carried
+; false dependency on an xmm that is modified by other scalar instructions
+; that follow it in the loop. Additionally, the source of convert is a 
+; memory operand. Verify the execution dependency fix pass breaks this
+; dependency by inserting a xor before the convert.
+@x = common global [1024 x double] zeroinitializer, align 16
+@y = common global [1024 x double] zeroinitializer, align 16
+@z = common global [1024 x double] zeroinitializer, align 16
+@w = common global [1024 x double] zeroinitializer, align 16
+@v = common global [1024 x i32] zeroinitializer, align 16
+
+define void @f3() {
+entry:
+  br label %for.cond1.preheader
+
+for.cond1.preheader:                              ; preds = %for.inc14, %entry
+  %i.025 = phi i32 [ 0, %entry ], [ %inc15, %for.inc14 ]
+  br label %for.body3
+
+for.body3:
+  %indvars.iv = phi i64 [ 0, %for.cond1.preheader ], [ %indvars.iv.next, %for.body3 ]
+  %arrayidx = getelementptr inbounds [1024 x i32]* @v, i64 0, i64 %indvars.iv
+  %0 = load i32* %arrayidx, align 4
+  %conv = sitofp i32 %0 to double
+  %arrayidx5 = getelementptr inbounds [1024 x double]* @x, i64 0, i64 %indvars.iv
+  %1 = load double* %arrayidx5, align 8
+  %mul = fmul double %conv, %1
+  %arrayidx7 = getelementptr inbounds [1024 x double]* @y, i64 0, i64 %indvars.iv
+  %2 = load double* %arrayidx7, align 8
+  %mul8 = fmul double %mul, %2
+  %arrayidx10 = getelementptr inbounds [1024 x double]* @z, i64 0, i64 %indvars.iv
+  %3 = load double* %arrayidx10, align 8
+  %mul11 = fmul double %mul8, %3
+  %arrayidx13 = getelementptr inbounds [1024 x double]* @w, i64 0, i64 %indvars.iv
+  store double %mul11, double* %arrayidx13, align 8
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 1024
+  br i1 %exitcond, label %for.inc14, label %for.body3
+
+for.inc14:                                        ; preds = %for.body3
+  %inc15 = add nsw i32 %i.025, 1
+  %exitcond26 = icmp eq i32 %inc15, 100000
+  br i1 %exitcond26, label %for.end16, label %for.cond1.preheader
+
+for.end16:                                        ; preds = %for.inc14
+  ret void
+
+;CHECK-LABEL:@f3
+;CHECK: xorps [[XMM0:%xmm[0-9]+]], [[XMM0]]
+;CHECK-NEXT: cvtsi2sdl {{.*}}, [[XMM0]]
+;CHECK-NEXT: mulsd {{.*}}, [[XMM0]]
+;CHECK-NEXT: mulsd {{.*}}, [[XMM0]]
+;CHECK-NEXT: mulsd {{.*}}, [[XMM0]]
+;CHECK-NEXT: movsd [[XMM0]],
+}