[PATCH] D97837: [InstCombine] Remove gc.relocate duplicates

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

LGTM w/minor comments.

I'm LGTMing this as the code appears correct, but I ask that you don't land this if the previously linked GVN patch solves the original test case.  I'm not completely against duplicating the CSE into InstCombine (specific for gc.relocates), but I'd rather avoid that duplication.  I also don't want to block progress indefinitely though.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2179
     SmallPtrSet<Value *, 32> LiveGcValues;
+    // Utilities to eliminate duplicated gc.relocate instruction.
+    // If there are two relocation with the same base/derived indexes then
----------------
Can you reword this comment enough to a) emphasize this is cse, and b) explain why we're doing this here in addition to gvn? 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2193
+      auto &DupSet = IdxToRel.FindAndConstruct(Key).second;
+      if (!DupSet.size()) {
+        DupSet.insert(GCR);
----------------
Please use .empty here, it's more idiomatic.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2215
+      DupSet.insert(GCR);
+      return ToErase.size() != 0;
+    };
----------------
again !empty


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97837/new/

https://reviews.llvm.org/D97837