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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/two required comments, and one style suggestion.

Please wait 24 hours in case anyone else has comments before landing.



================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:232
+      NumSpilledRegisters++;
+      RegToSlotIdx[Reg] = FI;
+    }
----------------
skatkov wrote:
> 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?
As a general point, the less "object state" there is, the easier the code is to maintain and understand.  Structures which are only used in a subset of member functions tend to be some of the most confusing.

You'd need more arguments to the free function than I first thought, so you can ignore the follow up thought.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:194
+        continue;
+      if (VisitedRegs.insert(Reg).second)
+        RegsToSpill.push_back(Reg);
----------------
VisitedRegs isn't used outside this function.  Reduce scope to local variable above loop.


================
Comment at: llvm/test/CodeGen/X86/statepoint-regs.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -O3 -restrict-statepoint-remat -use-registers-for-deopt-values < %s | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
I believe -restrict-statepoint-remat is the default.  If so, remove from test.


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

https://reviews.llvm.org/D77371