[PATCH] D9068: ScheduleDAGInstrs: Optionally respect lanemasks when creating dependencies.

[Quentin Colombet via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

Hi Matthias,

The change looks mostly good to me.
There is one FIXME though, that I believe should be fixed before pushing.

Thanks,
-Quentin


================
Comment at: include/llvm/CodeGen/ScheduleDAGInstrs.h:100
@@ +99,3 @@
+    /// Whether lane masks should get tracked.
+    bool TrackLaneMasks;
+
----------------
Should we mention the sub-register here?
I am wondering if it is clear for everybody that lane masks come from sub-registers.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:371
@@ +370,3 @@
+  if (!TrackLaneMasks)
+    return 1;
+  unsigned Reg = MO.getReg();
----------------
I guess the constant used here is not very important since we consistently have the same for every virtual registers.
That being said, it feels more natural to use ~0U, to emphasize we set the whole register, however I reckon we would set too many “lane mask” bits.
What do you think?

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:376
@@ +375,3 @@
+  if (!RC.HasDisjunctSubRegs)
+    return 1;
+
----------------
Ditto.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:402
@@ +401,3 @@
+    // earlier instruction.
+    unsigned KillLaneMask = MO.isUndef() ? ~0u : DefLaneMask;
+    // Add data dependence to all uses we found so far.
----------------
Didn’t we come to the conclusion that the other lanes may be undef for this use but still live?
I.e., the kill mask is redundant with the def mask.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:417
@@ +416,3 @@
+        SDep Dep(SU, SDep::Data, Reg);
+        unsigned UseOperIdx = 0; // TODO! FIXME! Get a proper value!
+        Dep.setLatency(SchedModel.computeOperandLatency(MI, OperIdx, Use,
----------------
When do you plan to fix this?
I am guessing there is not much point in pushing the patch forward without that fixed.


Repository:
  rL LLVM

http://reviews.llvm.org/D9068