[PATCH] D18106: Extract out a SelectionDAGBuilder::LowerAsStatepoint; NFC
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 11 18:38:57 PST 2016
reames added a comment.
A couple of minor comments for now. I'm going to return to this Monday. FYI, the organization of this is really bugging me. Hopefully, I'll be able to explain why with a bit more thought. :)
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:249
@@ +248,3 @@
+static void
+removeDuplicatesGCPtrs(SmallVectorImpl<const Value *> &Bases,
+ SmallVectorImpl<const Value *> &Ptrs,
----------------
minor: this looks like an unrelated NFCI change. Please separate and land to simplify review.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:345
@@ -346,3 +344,3 @@
// Export the result value if needed
- const Instruction *GCResult = ISP.getGCResult();
+ const Instruction *GCResult = SI.GCResult;
if (HasDef && GCResult) {
----------------
This is going to be confusing named if reused. For a non-statepoint call with deopt, the GCResult instruction will just be the instruction itself.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:375
@@ -377,1 +374,3 @@
+ } else if (SI.IsStatepoint) {
// The token value is never used from here on, just generate a poison value
+ Builder.setValue(SI.getInstruction(),
----------------
I think this is valid for a non-statepoint void call as well.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:392
@@ -393,3 +391,3 @@
SmallVectorImpl<const Value *> &Bases, SmallVectorImpl<const Value *> &Ptrs,
- SmallVectorImpl<const Value *> &Relocs, ImmutableStatepoint StatepointSite,
- SelectionDAGBuilder &Builder) {
+ SmallVectorImpl<const GCRelocateInst *> &Relocs,
+ ImmutableStatepoint StatepointSite, SelectionDAGBuilder &Builder) {
----------------
again, nfci and separate.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:580
@@ -592,3 +579,3 @@
// the alloca
- for (Value *V : StatepointSite.gc_args()) {
+ for (Value *V : SI.GCArgs) {
SDValue Incoming = Builder.getValue(V);
----------------
I think the only thing we're using GCArgs for is this loop. Might be better to just pass in the explicit set of spill slots.
http://reviews.llvm.org/D18106
More information about the llvm-commits
mailing list