[PATCH] D25096: [RS4GC] New pass to remove gc.relocates added by RS4GC

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:36
+  void getAnalysisUsage(AnalysisUsage &Info) const override {
+    Info.setPreservesAll();
+  }
----------------
Not sure if this is correct, since you're changing IR.  I'd suggest starting with "clobbers all" for now.


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:41
+
+  SmallVector<GCRelocateInst *, 20> GCRelocates;
+};
----------------
Why not have this just be a local that lives on stack in `runOnFunction`?


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:49
+  // Nothing to do for declarations.
+  if (F.isDeclaration() || F.empty())
+    return false;
----------------
I didn't know we could have empty non-declarations.


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:53
+  for (Instruction &I : instructions(F)) {
+    ImmutableCallSite GCRel = ImmutableCallSite(&I);
+    if (!isGCRelocate(GCRel))
----------------
I don't think you need this double staged casting -- you should be able to do:

```
if (auto *GCR = dyn_cast<GCRelocateInst>(&I))
  GCRelocates.push_back(GCR);
```



================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:57
+    GCRelocates.push_back(cast<GCRelocateInst>(&I));
+
+  }
----------------
Empty line?


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:70
+
+    Instruction *ToBeReplaced = GCRel;
+    // All gc_relocates are i8 addrspace(1)* typed, and may be bitcasted to
----------------
I wouldn't bother with this.  I'd just insert a bitcast of the original to `i8 addrspace(1)*`, RAUW with that.  We can always run `-instcombine` later to clean up the IR quickly.

At the very least, I'd do this ^ in place of the `continue` down below:

```
      if (GCRel->hasOneUse() && isa<BitCastInst>(GCRel->user_back()))
        ToBeReplaced = GCRel->user_back();
      else
        // Create new bitcast and RAUW with that.
```


https://reviews.llvm.org/D25096