[GC] Turn {undef,null} transforms into strategy

[artagnon]

Summary

The code doesn't seem to run for some reason. Hints?

[reames]

What are you trying to accomplish here? The patch appears slightly nonsensical. The lowering code your modifying is from the old gc.root mechanism and isn't involved with statepoints.

Why do you want to move the optimization code out of InstCombine? Arguably some of it should live in InstSimplify, but it's definitely an IR level optimization.

Do you have a collector which doesn't want these?

[artagnon]

I'm just following what I think I understood from your comment: "There might be some weird collector this property does not hold for." -- so, I thought there should be a way to define a custom lowering pass for "relocate undef" or "relocate null" instead of turning them into "undef" and "null" everytime. Did I misunderstand? What are those TODO comments about?

GCStrategy code is invoked on a gc "..." annotation, irrespective of whether it uses the gcroot mechanism, no?

[sanjoy]

I'm just following what I think I understood from your comment: "There might be some weird collector this property does not hold for." -- so, I thought there should be a way to define a custom lowering pass for "relocate undef" or "relocate null" instead of turning them into "undef" and "null" everytime. Did I misunderstand? What are those TODO comments about?

In this context: "There might be some weird collector this property
does not hold for." == "This property holds for virtually every
production-grade garbage collector". It is better to defer this
complexity (of conditionally switching off the two optimizations) till
someone actually tries to get statepoints working with such a
collector.

GCStrategy code is invoked on a gc "..." annotation, irrespective of whether it uses the gcroot mechanism, no?

relocate(null) == null is an optimization, similar to "add x 0 == x".
I don't think it should live in GCStrategy -- it is inaccurate for
this transform to be part of any "lowering" phase. Even from a
practical standpoint, it is better to run it interleaved with other
transforms done by instcombine.

• Sanjoy

[reames]

Sanjoy's response covered much of what I would otherwise say. One small
addition: If we do add a hook to the GCStrategy class to control these
optimizations, the location of the optimization should not change. The
intent of my "Move ownership of GCStrategy objects to LLVMContext"
change (up for review now) is to allow InstCombine and other passes to
get at properties of the GCStrategy in play.

Philip

include/llvm/CodeGen/GCStrategy.h

namespace llvm {
   private:
     std::string Name;
     friend class GCModuleInfo;
     
   protected:
     unsigned NeededSafePoints; ///< Bitmask of required safe points.
     bool CustomReadBarriers;   ///< Default is to insert loads.
     bool CustomWriteBarriers;  ///< Default is to insert stores.
+    bool CustomUndefRelocation;///< Default is to insert undef.
+    bool CustomNullRelocation; ///< Default is to insert null.
     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
     /// name string specified on functions which use this strategy.
     const std::string &getName() const { return Name; }
 
     /// By default, write barriers are replaced with simple store
     /// instructions. If true, then performCustomLowering must instead lower
     /// them. 
     bool customWriteBarrier() const { return CustomWriteBarriers; }
     
     /// By default, read barriers are replaced with simple load
     /// instructions. If true, then performCustomLowering must instead lower
     /// them. 
     bool customReadBarrier() const { return CustomReadBarriers; }
 
+    /// By default, undef values are undef after relocation. If true, then
+    /// performCustomLowering must instead lower them.
+    bool customUndefRelocation() const { return CustomUndefRelocation; }
+
+    /// By default, null values are null after relocation. If true, then
+    /// performCustomLowering must instead lower them.
+    bool customNullRelocation() const { return CustomNullRelocation; }
+
     /** @name GCRoot Specific Properties
      * 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. 

lib/CodeGen/GCStrategy.cpp

 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/Statepoint.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetFrameLowering.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
bool LowerIntrinsics::runOnFunction(Function &F) {
   }
 
   return MadeChange;
 }
 
 bool LowerIntrinsics::PerformDefaultLowering(Function &F, GCStrategy &S) {
   bool LowerWr = !S.customWriteBarrier();
   bool LowerRd = !S.customReadBarrier();
+  bool LowerUndef = !S.customUndefRelocation();
+  bool LowerNull = !S.customNullRelocation();
   bool InitRoots = S.initializeRoots();
 
   SmallVector<AllocaInst*, 32> Roots;
 
   bool MadeChange = false;
   for (Function::iterator BB = F.begin(), E = F.end(); BB != E; ++BB) {
     for (BasicBlock::iterator II = BB->begin(), E = BB->end(); II != E;) {
       if (IntrinsicInst *CI = dyn_cast<IntrinsicInst>(II++)) {
for (BasicBlock::iterator II = BB->begin(), E = BB->end(); II != E;) {
           break;
         case Intrinsic::gcroot:
           if (InitRoots) {
             // Initialize the GC root, but do not delete the intrinsic. The
             // backend needs the intrinsic to flag the stack slot.
             Roots.push_back(cast<AllocaInst>(
                               CI->getArgOperand(0)->stripPointerCasts()));
           }
-          break;
+    break;
+        case Intrinsic::experimental_gc_relocate:
+          if (LowerUndef) {
+            // Replace a relocation of an undef value with undef.
+      GCRelocateOperands Operands(II);
+      Value *DerivedPtr = Operands.derivedPtr();
+
+      if (isa<UndefValue>(DerivedPtr))
+        CI->replaceAllUsesWith(DerivedPtr);
+            CI->eraseFromParent();
+          }
+          if (LowerNull) {
+            // Replace a relocation of a null value with null.
+      GCRelocateOperands Operands(II);
+      Value *DerivedPtr = Operands.derivedPtr();
+
+      if (isa<ConstantPointerNull>(DerivedPtr))
+        CI->replaceAllUsesWith(DerivedPtr);
+            CI->eraseFromParent();
+          }
+          break;
         default:
           continue;
         }
 
         MadeChange = true;
       }
     }
   }

lib/Transforms/InstCombine/InstCombineCalls.cpp

case Intrinsic::experimental_gc_relocate: {
     GCRelocateOperands Operands(II);
     Value *DerivedPtr = Operands.derivedPtr();
 
     // Remove the relocation if unused, note that this check is required
     // to prevent the cases below from looping forever.
     if (II->use_empty())
       return EraseInstFromFunction(*II);
 
-    // Undef is undef, even after relocation.
-    // TODO: provide a hook for this in GCStrategy.  This is clearly legal for
-    // most practical collectors, but there was discussion in the review thread
-    // about whether it was legal for all possible collectors.
-    if (isa<UndefValue>(DerivedPtr))
-      return ReplaceInstUsesWith(*II, DerivedPtr);
-
-    // The relocation of null will be null for most any collector.
-    // TODO: provide a hook for this in GCStrategy.  There might be some weird
-    // collector this property does not hold for.
-    if (isa<ConstantPointerNull>(DerivedPtr))
-      return ReplaceInstUsesWith(*II, DerivedPtr);
-
     // isKnownNonNull -> nonnull attribute
     if (isKnownNonNull(DerivedPtr))
       II->addAttribute(AttributeSet::ReturnIndex, Attribute::NonNull);
 
     // TODO: dereferenceable -> deref attribute
 
     // TODO: bitcast(relocate(p)) -> relocate(bitcast(p))
     // Canonicalize on the type from the uses to the defs