[PATCH] D43093: [FastISel] Sink local value materializations to first use

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Reid,

The runtime needs to be checked but the overall approach look sensible for the problem at hand.
Now, should we go to such length to make the debug information be friendlier to some debugger, I am not sure, but I let you decide.

Random thoughts, inside of sinking the local values towards their first use, could we postpone emitting them until we finish selecting the block?

Cheers,
-Quentin



================
Comment at: llvm/include/llvm/CodeGen/FastISel.h:568
+  /// in the basic block. Returns the point at which the insertion occurred or
+  /// the null iterator.
+  void sinkLocalValueMaterialization(MachineInstr &LocalMI, unsigned DefReg);
----------------
This does not return anything. Please fix the comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:125
 void FastISel::startNewBlock() {
-  LocalValueMap.clear();
+  assert(LocalValueMap.empty());
 
----------------
Could you add a message in that assert please?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:160
+/// Return the defined register if this instruction defines exactly one
+/// register. Otherwise return 0.
+static unsigned findSinkableLocalRegDef(MachineInstr &MI) {
----------------
The comment doesn't match what the function is doing.
>From the comment alone, I would have expected that we don't look at uses.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:178
+
+// FIXME: Remove this after measuring -O0 compile time.
+static cl::opt<bool> FastISelSinkLocalValues(
----------------
@Hans gave you advices on how to do the measurements.

Do you still plan to push with that option?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:188
+  // instructions a useful DebugLoc without creating a jumpy, non-linear line
+  // table.
+  if (FastISelSinkLocalValues && LastLocalValue != EmitStartPt) {
----------------
I have mixed feelings about the approach.
I am sensible to the debug location thing, but taking care of avoiding spilling here is kind of sketchy.

I would rather we focus on one goal, and I believe that goal should be putting sensible debug location.

What I am saying is if we want to work on the spilling case, given every ISels have this problem (SDISel and GISel are also affected), we should probably improve the fast reg allocator instead of coming up with workaround everywhere.

Could be just a comment thing and we don't mention spilling (or say that the plus side is that it gives shorter live-ranges and that helps RegAllocFast).


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:195
+        EmitStartPt
+            ? std::next(MachineBasicBlock::reverse_iterator(EmitStartPt))
+            : FuncInfo.MBB->rend();
----------------
Shouldn't this be MachineBasicBlock::reverse_iterator(EmitStartPt)?
We don't want to go pass the EmitStartPt, do we?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:223
+    if (P.second == DefReg)
+      return true;
+  return false;
----------------
Would be more natural to walk through the uses of DefReg.

Do we need to have the PHI uses in that list for that check to be relevant?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:240
+  for (MachineInstr &UseInst : MRI.use_nodbg_instructions(DefReg))
+    UseSet.insert(&UseInst);
+  bool UsedByPHI = isRegUsedByPhiNodes(DefReg, FuncInfo);
----------------
We could postpone that build until after the phi check.
(The empty check does not need the full set to be performed.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:251
+    return;
+  }
+
----------------
I don't really like that we are doing DCE here.
I feel that either we should leave that to the rest of the pipeline or have a more centralized place in FastISel to do that. I mean I believe some other instructions can probably be DCE'ed so why do it here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:259
+  for (auto II = std::next(LocalMI.getIterator()),
+            EE = FuncInfo.MBB->instr_end();
+       II != EE; ++II) {
----------------
That loop could be fairly expensive.
Assuming we have more than one local value to sink, we could number the instructions and just walk through all the users of the value and keep the one with the lowest number.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:264
+    bool StopAtTerminatorOrLabel =
+        UsedByPHI && (II->isEHLabel() || II->isTerminator());
+    if (StopAtTerminatorOrLabel || UseSet.count(&*II)) {
----------------
I don't get why we use UsedByPhi here.
We shouldn't insert after the first terminator no matter what and this is not properly checked here.


https://reviews.llvm.org/D43093