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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked 14 inline comments as done.
skatkov added inline comments.


================
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.
----------------
reames wrote:
> 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.  
I've tried to re-phrase, please take a look in new version.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:154
+  // Add size of register to cache.
+  void addRegSize(Register Reg) {
+    if (RegToSpillSize.count(Reg))
----------------
reames wrote:
> 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.
ok, let's get rid of that.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:218
+      Ops.push_back(Idx);
+      CacheFI.addRegSize(Reg);
+    }
----------------
reames wrote:
> For a reg used multiple times, you're calling addRegSize multiple times.
Yes, no problem with that, but as soon as I'm eliminating addRegSize it does not make sense anymore.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:232
+      NumSpilledRegisters++;
+      RegToSlotIdx[Reg] = FI;
+    }
----------------
reames wrote:
> 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.
I do not follow what you are trying to reach.
Yes, we can extract rewriteStatepoint from StatepointState and provide as an arguments MI, TRI, TII, MF, MFI, RegToSlotIdx and OpsToSpill.

What is an idea behind this?


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:251
+        MIB.addImm(StackMaps::IndirectMemRefOp);
+        MIB.addImm(MFI.getObjectSize(FI));
+        assert(MO.isReg() && "Should be register");
----------------
reames wrote:
> I don't know that this line is correct.  Don't you want the original register size?
For FixupSCSExtendSlotSize == false it does not matter (size will be the same)
For true case... I do not have a strong preference. It depends on how runtime will determine the size of the value to read.
Let's change this to original register size.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:267
+      auto *MMO = MF.getMachineMemOperand(PtrInfo, MachineMemOperand::MOLoad,
+                                          MFI.getObjectSize(FrameIndex),
+                                          MFI.getObjectAlign(FrameIndex));
----------------
Here as well


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:322
+      if (I.getOpcode() == TargetOpcode::STATEPOINT)
+        Statepoints.push_back(&I);
+
----------------
reames wrote:
> 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)
To be honest I do not see any real reason to reduce the size of this list.
LiveIn are filtered out as a first step of process routine.

If you really want to see this list to be small I can move the logic of filtering out LiveIns from process to here.
One minor disadvantage of it would be to trigger StatepointOpers(&MI).getVarIdx() twice for non LiveIns statepoints.

If you believe moving the logic here is more clear, let me know and I do it.



================
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,
----------------
reames wrote:
> 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)
https://reviews.llvm.org/D77629


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:916
 
+  if (UseRegistersForStatepoint)
+    addPass(&FixupStatepointCallerSavedID);
----------------
reames wrote:
> Can we just run this unconditionally?
Yes, but it will require to update tests which check pipeline.
I would propose to do it as follow-up.


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

https://reviews.llvm.org/D77371