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

[Keno Fischer via Phabricator via llvm-commits]

loladiro added inline comments.


================
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
----------------
myatsina wrote:
> 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.
Without D28915 there several hundred more xorps to be inserted in the tests ;)



================
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
----------------
myatsina wrote:
> 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.
Yes, this relies on the optimization in D28915. With this patch, but without the other one it would have inserted three xorps here (one for each of xmm2, xmm3, xmm4). The optimization in that patch allows us to get away with one.

The reason this changed here and not in the other patch is that before this, xmm2 was considered to have sufficient clearance (same for xmm3 and xmm4), but now it doesn't any more.


================
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
----------------
myatsina wrote:
> Why the change? What are you testing here?
As remarked in the other revision, this is a trick for making sure that pickBestRegister is still tested.


================
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
----------------
myatsina wrote:
> 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).
> 
What kind of inline asm do you have in mind for this purpose? I tried for a bit to come up with something, but didn't manage to.


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


================
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
----------------
myatsina wrote:
> why the change to xmm2?
> There is a dependency on it "really close", 3 instructions above it (vpcmpgtq %xmm1, %xmm0, %xmm2)
Well, there needs to be an xor with some register here. xmm2 seems as good as any other, unless the fact that is was used shortly before actually makes a difference (I'm not aware of such an impact, since this should be handle in the register renaming unit, but happy to be corrected). 


Repository:
  rL LLVM

https://reviews.llvm.org/D28786