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

[Reid Kleckner via Phabricator via llvm-commits]

rnk planned changes to this revision.
rnk marked 3 inline comments as done.
rnk added a comment.

Thanks, I will implement the instruction numbering change tomorrow and then get new numbers. I'll upload a fresh patch in the meantime, though.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:178
+
+// FIXME: Remove this after measuring -O0 compile time.
+static cl::opt<bool> FastISelSinkLocalValues(
----------------
qcolombet wrote:
> @Hans gave you advices on how to do the measurements.
> 
> Do you still plan to push with that option?
I think I do. If someone complains that they find some performance regression, I want to be able to say "try flipping this flag, see if it gets faster".


================
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) {
----------------
qcolombet wrote:
> 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).
OK, I revised it to this:
  // Try to sink local values down to their first use so that we can give them a
  // better debug location. This has the side effect of shrinking local value
  // live ranges, which helps out fast regalloc.

So, basically it's about shrinking live ranges. Avoiding spills is the regalloc's job, not ours.


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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:223
+    if (P.second == DefReg)
+      return true;
+  return false;
----------------
qcolombet wrote:
> 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?
These PHI uses do not appear in the use list because we are in the middle of ISel. We haven't formed the PHI instructions or their operand lists yet.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:259
+  for (auto II = std::next(LocalMI.getIterator()),
+            EE = FuncInfo.MBB->instr_end();
+       II != EE; ++II) {
----------------
qcolombet wrote:
> 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.
That's a really good idea, I'll do that.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:264
+    bool StopAtTerminatorOrLabel =
+        UsedByPHI && (II->isEHLabel() || II->isTerminator());
+    if (StopAtTerminatorOrLabel || UseSet.count(&*II)) {
----------------
qcolombet wrote:
> 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.
My intention was to avoid sinking across EH_LABELs when the value is used by a PHI. Stopping at terminators unconditionally makes sense, but it weakens the assertion below that we must find some use of a local value that isn't used by a PHI.


https://reviews.llvm.org/D43093