[PATCH] D28786: [ExecutionDepsFix] Kill clearance at function entry/calls

[Marina Yatsina via Phabricator via llvm-commits]

myatsina added a comment.

The existent tests you've changes seem to be affect by other changes as well.
Can you please upload only the changed tests' logic that is affected by this patch alone?
I would not like to see the affects of "xor fusion" or the other things here, just the support for call instructions in clearance calculation



================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:412
+    // here seems worth it.
+    for (unsigned rx = 0; rx != NumRegs; ++rx) {
+      LiveRegs[rx].Def = -1;
----------------
I was about to upload a review for this issue :)
It solves at least 2 bugzillas:
https://llvm.org/bugs/show_bug.cgi?id=25277
https://llvm.org/bugs/show_bug.cgi?id=27573

One of them is marked as duplicate of other issues, so it will solve those too probably.

You should mention these bugzillas in your commit message.


In my version I had some small refactoring to this code section, feel free to adopt it if you like it:

  // If this is an entery block, we don't know what the caller function did
  // with the register, therfore we treat them as they were defined just before
  // the first instruction.
  // Otherwise, default values are 'nothing happened a long time ago'.
  int defaultDefVal = MBB->pred_empty() ? -1 : -(1 << 20);

   for (unsigned rx = 0; rx != NumRegs; ++rx) {
     LiveRegs[rx].Value = nullptr;
     LiveRegs[rx].Def = defaultDefVal;
   }

   // This is the entry block.
   if (MBB->pred_empty()) {
     DEBUG(dbgs() << "BB#" << MBB->getNumber() << ": entry\n");
     return;
   }


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:631
     }
+  } else if (MI->isCall()) {
+    // If this is a call, pretend all registers we are considering are def'd
----------------
Here, you've made this version dependent on the D28915 review.
I suggest you commit this patch before D28915, but then you need to change this condition a bit (else if --> if).

You can change it back in the "isDependencyBreak" review.


================
Comment at: test/CodeGen/X86/avx512-cvt.ll:19
 ; KNL-NEXT:    vpextrq $1, %xmm1, %rax
-; KNL-NEXT:    vcvtsi2sdq %rax, %xmm2, %xmm2
+; KNL-NEXT:    vxorps %xmm4, %xmm4, %xmm4
+; KNL-NEXT:    vcvtsi2sdq %rax, %xmm4, %xmm2
----------------
I think the checks in this test mix some of of your separate changes (the "XOR" fusion?).
Are you sure this tst is correct? Shouldn't it have 2 xors? also, I'm not sure why xmm2, xmm3 changed to xmm4 here.


================
Comment at: test/CodeGen/X86/avx512-cvt.ll:60
+; KNL-NEXT:    vxorps %xmm3, %xmm3, %xmm3
+; KNL-NEXT:    vcvtsi2sdq %rax, %xmm3, %xmm2
 ; KNL-NEXT:    vmovq %xmm1, %rax
----------------
Same here and in the rest of this file, I don;t understand why the register changed from xmm2 to xmm3. This should not have been caused by the change in this patch.


================
Comment at: test/CodeGen/X86/break-false-dep.ll:269
+;AVX-NEXT: vucomiss [[XMM4_7]], %xmm0
+  %0 = fcmp ult float %z, 0.0
+  br i1 %0, label %loop, label %fake
----------------
Why the change? What are you testing here?


================
Comment at: test/CodeGen/X86/break-false-dep.ll:362
+
+; Make sure that calls kill register clearance and that a we don't insert
+; an extra dependency-breaking instruction if one suffices.
----------------
let's separate between xor after call instruction and "xor fusion". these are 2 different features and each should have it's own tests, and this is the exact purpose of unit tests.
If you want a test that combines several feature it shouldn't come at the expense of each feature's stand alone tests. It can be added on top of them.


================
Comment at: test/CodeGen/X86/break-false-dep.ll:364
+; an extra dependency-breaking instruction if one suffices.
+declare double @sin(double %x)
+define void @callclearance(double *%x, i64 *%y, i64 *%z) {
----------------
This change should contain 2 tests,
1. Check xor in the callee after a call (which callclearance does)
2. Check xor in a function (I know it is indirectly tests in a lot of other places, but I think we should have an explicit test for this).


================
Comment at: test/CodeGen/X86/break-false-dep.ll:371
+  %idx = phi i32 [0, %entry], [%idx, %loop]
+  %valptr = getelementptr i64, i64* %y, i32 %idx
+  %valptr2 = getelementptr i64, i64* %z, i32 %idx
----------------
A suggestion for a simpler test that tests the call feature:

define <4 x double> @callclearance( i64 %val ) {
// inline asm to force choosing one register
 %val1 = sitofp i64 %val to double
 %val2 = sitofp i64 %val to double
  %2 = call <4 x double> @myfunc(i64 %val, i64 %val1)
 %val3 = sitofp i64 %val to double
...
}

Show that the undef register of the first and the second convert are the same, but the undef register of the third convert is different from them (--> the call instruction changes the clearance and thus another register was chosen).



================
Comment at: test/CodeGen/X86/half.ll:104
 ; CHECK-LIBCALL-NEXT: movq %rsi, [[ADDR]]
+; CHECK-LIBCALL-NEXT: xorps
 ; CHECK-LIBCALL-NEXT: cvtsi2ssq %rdi, %xmm0
----------------
the register is written explicitly, so better check the register too, no?
There are other tests like this further on.


================
Comment at: test/CodeGen/X86/i64-to-float.ll:281
 ; X64-AVX-NEXT:    vpextrq $1, %xmm0, %rax
-; X64-AVX-NEXT:    vcvtsi2sdq %rax, %xmm3, %xmm1
+; X64-AVX-NEXT:    vxorps %xmm2, %xmm2, %xmm2
+; X64-AVX-NEXT:    vcvtsi2sdq %rax, %xmm2, %xmm1
----------------
why the change to xmm2?
There is a dependency on it "really close", 3 instructions above it (vpcmpgtq %xmm1, %xmm0, %xmm2)


Repository:
  rL LLVM

https://reviews.llvm.org/D28786