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

[Anna Thomas via llvm-commits]

anna marked an inline comment as done.
anna added inline comments.


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:49
+  // Nothing to do for declarations.
+  if (F.isDeclaration() || F.empty())
+    return false;
----------------
sanjoy wrote:
> I didn't know we could have empty non-declarations.
Actually, I think there cannot be empty non-declarations after all optimizations are done. 
Either way the pass will not do anything for empty functions. Removed that check.


================
Comment at: lib/Transforms/Utils/RemoveGCRelocates.cpp:70
+
+    Instruction *ToBeReplaced = GCRel;
+    // All gc_relocates are i8 addrspace(1)* typed, and may be bitcasted to
----------------
sanjoy wrote:
> 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.
> ```
Yes, the second idea was what I was thinking of originally, and mentioned in comment below.

I like your first idea better, the code is cleaner, and instcombine would get rid of unnecessary bitcasts (if any). 


https://reviews.llvm.org/D25096