Remove the gc.root's findCustomSafePoints mechanism

[reames]

Summary

Searching all of the existing gc.root implementations I'm aware of (all three of them), there was exactly one use of this mechanism, and that was to implement a performance improvement that should have been applied to the default lowering.

Having this function is requiring a dependency on a CodeGen class (MachineFunction), in a class which is otherwise completely independent of CodeGen. I could solve this differently, but given that I see absolutely no value in preserving this mechanism, I'd like to just get rid of it.

The code change is pretty straight forward, but I wanted to get input before submission because this is the first time I'm intentionally breaking previously supported gc.root functionality. Given 3.6 has branched, I believe this is a good time to do this.

This is blocking an update to http://reviews.llvm.org/D6811

[whitequark]

lib/CodeGen/ErlangGC.cpp

59

Do I understand it correctly that the GC_LABEL MI will be inserted elsewhere for this safepoint?

[reames]

Not quite sure what you're asking...

The behaviour of the ErlangGC did not change. It's still using
post-call labels. (Or at least, I didn't *intend* to change it. If I
did, please point it out.) It's just reaching that result through the
(slightly modified) generic codepath.

[whitequark]

Ok, I see. LGTM.

include/llvm/CodeGen/GCStrategy.h

protected:
     bool UseStatepoints;       /// Uses gc.statepoints as opposed to gc.roots,
                                /// if set, none of the other options can be
                                /// anything but their default values.
 
     unsigned NeededSafePoints; ///< Bitmask of required safe points.
     bool CustomReadBarriers;   ///< Default is to insert loads.
     bool CustomWriteBarriers;  ///< Default is to insert stores.
     bool CustomRoots;          ///< Default is to pass through to backend.
-    bool CustomSafePoints;     ///< Default is to use NeededSafePoints
-                               ///< to find safe points.
     bool InitRoots;            ///< If set, roots are nulled during lowering.
     bool UsesMetadata;         ///< If set, backend must emit metadata tables.
     
   public:
     GCStrategy();
     virtual ~GCStrategy() {}
     
     /// Return the name of the GC strategy.  This is the value of the collector
public:
      * These properties and overrides only apply to collector strategies using
      * GCRoot. 
      */
     ///@{
 
     /// True if safe points of any kind are required. By default, none are
     /// recorded. 
     bool needsSafePoints() const {
-      return CustomSafePoints || NeededSafePoints != 0;
+      return NeededSafePoints != 0;
     }
     
     /// True if the given kind of safe point is required. By default, none are
     /// recorded. 
     bool needsSafePoint(GC::PointKind Kind) const {
       return (NeededSafePoints & 1 << Kind) != 0;
     }
         
     /// By default, roots are left for the code generator so it can generate a
     /// stack map. If true, then performCustomLowering must delete them.
     bool customRoots() const { return CustomRoots; }
 
-    /// By default, the GC analysis will find safe points according to
-    /// NeededSafePoints. If true, then findCustomSafePoints must create them.
-    bool customSafePoints() const { return CustomSafePoints; }
-    
     /// If set, gcroot intrinsics should initialize their allocas to null
     /// before the first use. This is necessary for most GCs and is enabled by
     /// default. 
     bool initializeRoots() const { return InitRoots; }
     
     /// If set, appropriate metadata tables must be emitted by the back-end
     /// (assembler, JIT, or otherwise). For statepoint, this method is
     /// currently unsupported.  The stackmap information can be found in the
public:
     virtual bool initializeCustomLowering(Module &F) {
       // No changes made
       return false;
     }
     virtual bool performCustomLowering(Function &F) {
       llvm_unreachable("GCStrategy subclass specified a configuration which"
                        "requires a custom lowering without providing one");
     }
-    ///@}
-    /// Called if customSafepoints returns true, used only by gc.root
-    /// implementations. 
-    virtual bool findCustomSafePoints(GCFunctionInfo& FI, MachineFunction& MF) {
-      llvm_unreachable("GCStrategy subclass specified a configuration which"
-                       "requests custom safepoint identification without"
-                       "providing an implementation for such");
-    }
   };
 
   /// Subclasses of GCStrategy are made available for use during compilation by
   /// adding them to the global GCRegistry.  This can done either within the
   /// LLVM source tree or via a loadable plugin.  An example registeration
   /// would be:
   /// static GCRegistry::Add<CustomGC> X("custom-name",
   ///        "my custom supper fancy gc strategy");
   /// 
   /// Note that to use a custom GCMetadataPrinter w/gc.roots, you must also
   /// register your GCMetadataPrinter subclass with the
   /// GCMetadataPrinterRegistery as well. 
   typedef Registry<GCStrategy> GCRegistry;
 }
 
 #endif

lib/CodeGen/ErlangGC.cpp

 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 
 using namespace llvm;
 
 namespace {
 
   class ErlangGC : public GCStrategy {
-    MCSymbol *InsertLabel(MachineBasicBlock &MBB,
-                          MachineBasicBlock::iterator MI,
-                          DebugLoc DL) const;
   public:
     ErlangGC();
-    bool findCustomSafePoints(GCFunctionInfo &FI, MachineFunction &MF) override;
   };
 
 }
 
 static GCRegistry::Add<ErlangGC>
 X("erlang", "erlang-compatible garbage collector");
 
 void llvm::linkErlangGC() { }
 
 ErlangGC::ErlangGC() {
   InitRoots = false;
   NeededSafePoints = 1 << GC::PostCall;
   UsesMetadata = true;
   CustomRoots = false;
-  CustomSafePoints = true;
-}
-
-MCSymbol *ErlangGC::InsertLabel(MachineBasicBlock &MBB,
-                                MachineBasicBlock::iterator MI,
-                                DebugLoc DL) const {
-  const TargetInstrInfo *TII = MBB.getParent()->getSubtarget().getInstrInfo();
-  MCSymbol *Label = MBB.getParent()->getContext().CreateTempSymbol();
-  BuildMI(MBB, MI, DL, TII->get(TargetOpcode::GC_LABEL)).addSym(Label);
-  return Label;
-}
-
-bool ErlangGC::findCustomSafePoints(GCFunctionInfo &FI, MachineFunction &MF) {
-  for (MachineFunction::iterator BBI = MF.begin(), BBE = MF.end(); BBI != BBE;
-       ++BBI)
-    for (MachineBasicBlock::iterator MI = BBI->begin(), ME = BBI->end();
-         MI != ME; ++MI)
-
-      if (MI->getDesc().isCall()) {
-
-        // Do not treat tail call sites as safe points.
-        if (MI->getDesc().isTerminator())
-          continue;
-
-        /* Code copied from VisitCallPoint(...) */
-        MachineBasicBlock::iterator RAI = MI; ++RAI;
-        MCSymbol* Label = InsertLabel(*MI->getParent(), RAI, MI->getDebugLoc());
-        FI.addSafePoint(GC::PostCall, Label, MI->getDebugLoc());
-      }
-
-  return false;
 }

lib/CodeGen/GCRootLowering.cpp

   }
 }
 
 void GCMachineCodeAnalysis::FindSafePoints(MachineFunction &MF) {
   for (MachineFunction::iterator BBI = MF.begin(), BBE = MF.end(); BBI != BBE;
        ++BBI)
     for (MachineBasicBlock::iterator MI = BBI->begin(), ME = BBI->end();
          MI != ME; ++MI)
-      if (MI->isCall())
+      if (MI->isCall()) {
+        // Do not treat tail or sibling call sites as safe points.  This is
+        // legal since any arguments passed to the callee which live in the
+        // remnants of the callers frame will be owned and updated by the
+        // callee if required.
+        if (MI->isTerminator())
+          continue;
         VisitCallPoint(MI);
+      }
 }
 
 void GCMachineCodeAnalysis::FindStackOffsets(MachineFunction &MF) {
   const TargetFrameLowering *TFI = TM->getSubtargetImpl()->getFrameLowering();
   assert(TFI && "TargetRegisterInfo not available!");
 
   for (GCFunctionInfo::roots_iterator RI = FI->roots_begin();
        RI != FI->roots_end();) {

   TM = &MF.getTarget();
   MMI = &getAnalysis<MachineModuleInfo>();
   TII = TM->getSubtargetImpl()->getInstrInfo();
 
   // Find the size of the stack frame.
   FI->setFrameSize(MF.getFrameInfo()->getStackSize());
 
   // Find all safe points.
-  if (FI->getStrategy().customSafePoints()) {
-    FI->getStrategy().findCustomSafePoints(*FI, MF);
-  } else {
-    FindSafePoints(MF);
-  }
+  FindSafePoints(MF);
 
   // Find the stack offsets for all roots.
   FindStackOffsets(MF);
 
   return false;
 }

lib/CodeGen/GCStrategy.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/GCStrategy.h"
 
 using namespace llvm;
 
 GCStrategy::GCStrategy()
     : UseStatepoints(false), NeededSafePoints(0), CustomReadBarriers(false),
-      CustomWriteBarriers(false), CustomRoots(false), CustomSafePoints(false),
+      CustomWriteBarriers(false), CustomRoots(false),
       InitRoots(true), UsesMetadata(false) {}

lib/CodeGen/StatepointExampleGC.cpp

public:
   StatepointGC() {
     UseStatepoints = true;
     // These options are all gc.root specific, we specify them so that the
     // gc.root lowering code doesn't run.
     InitRoots = false;
     NeededSafePoints = 0;
     UsesMetadata = false;
     CustomRoots = false;
-    CustomSafePoints = false;
   }
   Optional<bool> isGCManagedPointer(const Value *V) const override {
     // Method is only valid on pointer typed values.
     PointerType *PT = cast<PointerType>(V->getType());
     // For the sake of this example GC, we arbitrarily pick addrspace(1) as our
     // GC managed heap.  We know that a pointer into this heap needs to be
     // updated and that no other pointer does.
     return (1 == PT->getAddressSpace());