[PATCH] D77371: [Codegen/Statepoint] Allow usage of registers for non gc deopt values.

[Philip Reames via Phabricator via llvm-commits]

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

Generally seems like a reasonable direction to me, expect this to converge quickly.



================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:11
+/// \file
+/// Statepoint instruction in deopt parameters contains values which is
+/// meaningful to the runtime and should be able to read it.
----------------
A suggestion on how to rephrase this comment.

The key fact we need to encode is that might be a "late read" of the deopt value up to the moment the call returns.  If the register allocator could describe such a fact, it would produce the right form for us.  The need to fixup (i.e. this pass) is specifically handling the fact that we can't describe such a late read for the register allocator.

I'm using the term "late read" as an analogy to the "early clobber" concept that does exist.  


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:41
+#include "llvm/ADT/Statistic.h"
+#include "llvm/CodeGen/TargetFrameLowering.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
----------------
Order of includes


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:140
+      // spill slot size.
+      if (FixupSCSExtendSlotSize && MFI.getObjectSize(FI) < Size) {
+        MFI.setObjectSize(FI, Size);
----------------
You can remove the flag check here, as it's implied from the above.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:154
+  // Add size of register to cache.
+  void addRegSize(Register Reg) {
+    if (RegToSpillSize.count(Reg))
----------------
I don't understand why you bother to cache this.  I expect the lookup to be fast, am I wrong?

I could see having a local data structure in the sort routine, maybe, but I don't see the value outside of that.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:166
+      return;
+    std::sort(Regs.begin(), Regs.end(), [&](Register &A, Register &B) {
+      return RegToSpillSize[A] > RegToSpillSize[B];
----------------
llvm::sort?


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:186
+  // Operands with physical registers requiring spilling.
+  SmallVector<unsigned, 8> Ops;
+  // Set of register to spill.
----------------
OpsToSpill would be more clear.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:218
+      Ops.push_back(Idx);
+      CacheFI.addRegSize(Reg);
+    }
----------------
For a reg used multiple times, you're calling addRegSize multiple times.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:232
+      NumSpilledRegisters++;
+      RegToSlotIdx[Reg] = FI;
+    }
----------------
>From the look of it, RegToSlotIdx doesn't need to be a member variable and could instead be an out param of this function, and an in param of the next.  Once that's done, it looks like rewriteStatepoint is nearly a free function.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:251
+        MIB.addImm(StackMaps::IndirectMemRefOp);
+        MIB.addImm(MFI.getObjectSize(FI));
+        assert(MO.isReg() && "Should be register");
----------------
I don't know that this line is correct.  Don't you want the original register size?


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:322
+      if (I.getOpcode() == TargetOpcode::STATEPOINT)
+        Statepoints.push_back(&I);
+
----------------
To keep the size of this list small in practice, you might want to filter out LiveIn case here.  (i.e. side exits will dominate number of calls)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:65
+cl::opt<bool> UseRegistersForStatepoint(
+    "use-register-for-statepoint", cl::Hidden, cl::init(false),
+    cl::desc("Allow using registers for non pointer deopt args"));
----------------
Given we're planning to move on from this to gc values, I think we need a more descriptive name.
"use-register-for-deopt-values"?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:553
     const Value *Ptr = SI.Ptrs[i];
-    lowerIncomingStatepointValue(Builder.getValue(Ptr), /*LiveInOnly*/ false,
-                                 Ops, MemRefs, Builder);
+    lowerIncomingStatepointValue(Builder.getValue(Ptr),
+                                 /*RequireSpillSlot*/ true, Ops, MemRefs,
----------------
Everything in this file with the exception of the flag addition and one new comment is NFC.  Can you separate, commit separately, and rebase?

(i.e. the rename and refactor of the RequireSpillSlot logic is NFC)


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:916
 
+  if (UseRegistersForStatepoint)
+    addPass(&FixupStatepointCallerSavedID);
----------------
Can we just run this unconditionally?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77371/new/

https://reviews.llvm.org/D77371