[PATCH] D28915: [ExecutionDepsFix] Optimize instruction insertion

[Keno Fischer via Phabricator via llvm-commits]

loladiro added a comment.

I'll split up this review further and let's go from there.



================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:499
 /// is truly dependent on, or use a register with clearance higher than Pref.
 void ExeDepsFix::pickBestRegisterForUndef(MachineInstr *MI, unsigned OpIdx,
+                                          unsigned Pref, bool &TrueDependency) {
----------------
myatsina wrote:
> Why not make it return true/false (true dependency) instead of void?
> 
That seems fine. I think I originally had it return a value, which is why I went this way, but since that's no longer the case, I'll update accordingly.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:628
+      // issues, reset the counter.
+      LiveRegs[rx].Def = -(1 << 20);
+    }
----------------
myatsina wrote:
> This feature should be heavily tested.
> This is the "xor fusion" logic, right?
> 
> We're adding xors for dependency break in a late stage of the program, after clearance calculation of everyone was already done, including loops (i.e. you're previous patch that changes the calculation algorithm that you've comitted).
> Now you are changing the clearance for the xor's we've added, if I understand this correctly.
> So somehow you need to update your old clearance calculation, no?
> Or is this code aimed to only catch "xor"s that were in the original code.
> 
> I would like to see a lot of tests for these feature (without the affect of the other features):
> How does it affect simple one BB code?
> What is the affect on simple loops with one xor to this register and no dependency?
> What is the affect on complex loops that no xors and assigns to the same register:
> 
> loop:
> 
> cvt
> xor xmm0
> cvt
> xmm0 = 
> cvt
> cvt
> xor xmm0
> cvt
> xmm0 = 
This is just for xors that were in the original code. xor fusion is no longer there, because it got subsumed by the register re-picking.


https://reviews.llvm.org/D28915