[PATCH] D76013: [GC] Loosen ordering on statepoint reloads to allow CSE

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: skatkov, dantrushin, anna.
Herald added subscribers: bollu, hiraditya, mcrosier.
Herald added a project: LLVM.

We just removed a broken duplicate elimination algorithm in D75964 <https://reviews.llvm.org/D75964>, and after landed that it occurred to me that duplicate elimination is simply CSE.  SelectionDAG has a build in CSE, so why wasn't that triggering?  Well, it turns out we were overly conservative in the memory states for our reloads and CSE (rightly) considers the incoming memory state for a load part of the identity of the load.

By loosening the chain and allowing reordering, we also allow CSE.  As shown in the test case, doing iterative CSE as we go is enough to eliminate duplicate stores in later statepoints as well.  We key our (block local) slot map by SDValue, so commoning a previous pair of loads at construction time means we also common following stores.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76013

Files:
  llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
  llvm/test/CodeGen/X86/statepoint-duplicates-export.ll


Index: llvm/test/CodeGen/X86/statepoint-duplicates-export.ll
===================================================================
--- llvm/test/CodeGen/X86/statepoint-duplicates-export.ll
+++ llvm/test/CodeGen/X86/statepoint-duplicates-export.ll
@@ -44,19 +44,16 @@
 define i1 @test2(i32 addrspace(1)* %arg) gc "statepoint-example" {
 ; CHECK-LABEL: test2:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    subq $24, %rsp
-; CHECK-NEXT:    .cfi_def_cfa_offset 32
-; CHECK-NEXT:    movq %rdi, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movq %rdi, (%rsp)
 ; CHECK-NEXT:    callq func
 ; CHECK-NEXT:  .Ltmp2:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rax
-; CHECK-NEXT:    movq %rax, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    callq func
 ; CHECK-NEXT:  .Ltmp3:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rax
-; CHECK-NEXT:    orq {{[0-9]+}}(%rsp), %rax
+; CHECK-NEXT:    cmpq $0, (%rsp)
 ; CHECK-NEXT:    sete %al
-; CHECK-NEXT:    addq $24, %rsp
+; CHECK-NEXT:    popq %rcx
 ; CHECK-NEXT:    .cfi_def_cfa_offset 8
 ; CHECK-NEXT:    retq
 entry:
Index: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -973,10 +973,13 @@
   unsigned Index = *DerivedPtrLocation;
   SDValue SpillSlot = DAG.getTargetFrameIndex(Index, getFrameIndexTy());
 
-  // Note: We know all of these reloads are independent, but don't bother to
-  // exploit that chain wise.  DAGCombine will happily do so as needed, so
-  // doing it here would be a small compile time win at most.
-  SDValue Chain = getRoot();
+  // All the reloads are independent and are reading memory only modified by
+  // statepoints (i.e. no other aliasing stores); informing SelectionDAG of
+  // this this let's CSE kick in for free and allows reordering of instructions
+  // if possible.  The lowering for statepoint sets the root, so this is
+  // ordering all reloads with the either a) the statepoint node itself, or b)
+  // the entry of the current block for an invoke statepoint.
+  const SDValue Chain = DAG.getRoot(); // != Builder.getRoot()
 
   auto &MF = DAG.getMachineFunction();
   auto &MFI = MF.getFrameInfo();
@@ -991,8 +994,7 @@
 
   SDValue SpillLoad = DAG.getLoad(LoadVT, getCurSDLoc(), Chain,
                                   SpillSlot, LoadMMO);
-
-  DAG.setRoot(SpillLoad.getValue(1));
+  PendingLoads.push_back(SpillLoad.getValue(1));
 
   assert(SpillLoad.getNode());
   setValue(&Relocate, SpillLoad);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76013.249688.patch
Type: text/x-patch
Size: 2622 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200311/8b403ec4/attachment.bin>