[PATCH] D12376: Remove Merge Functions pointer comparisons

[JF Bastien via llvm-commits]

jfb added a comment.

Missing range metadata tests :-)


================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:536
@@ +535,3 @@
+int FunctionComparator::cmpRangeMetadata(const MDNode* L,
+                                         const MDNode* R) const {
+  if (L == R)
----------------
Could you add a TODO that explains how one could merge functions with different range metadata? The comparison could ignore it, and merging would take the most conservative of the ranges (widest interval, taking caution if there's wrapping, and deleting range information if the range becomes full/emtpy, merging pairs, ...).

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:543
@@ +542,3 @@
+    return 1;
+  // Range metadata should have two operands, both should be constant ints
+  assert(L->getNumOperands() == 2 && R->getNumOperands() == 2);
----------------
Range metadata has *pairs* of integers. It has at least two, but can have more:
http://llvm.org/docs/LangRef.html#range-metadata

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:544
@@ +543,3 @@
+  // Range metadata should have two operands, both should be constant ints
+  assert(L->getNumOperands() == 2 && R->getNumOperands() == 2);
+  ConstantInt* LLow = mdconst::extract<ConstantInt>(L->getOperand(0));
----------------
No need for the asserts, since `getOperand` already asserts.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:728
@@ +727,3 @@
+        return 0;
+      for(auto I = F->begin(), IE = F->end(); I != IE; ++I) {
+        if (&*I == LBB) {
----------------
Use a range-based `for` with `F->getBasicBlockList()`.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:737
@@ +736,3 @@
+      llvm_unreachable("Basic Block Address does not point to a basic block in "
+                       "its function.");
+    } else {
----------------
Need a `return` here for MSVC.

================
Comment at: test/Transforms/MergeFunc/merge-block-address-other-function.ll:6
@@ +5,3 @@
+
+; Function Attrs: nounwind uwtable
+define i32 @_Z1fi(i32 %i) #0 {
----------------
Remove the auto-generated comments and metadata from the tests (e.g. the `#0` below).

================
Comment at: test/Transforms/MergeFunc/merge-block-address.ll:56
@@ +55,3 @@
+; CHECK-LABEL: define i32 @_Z1gi
+; CHECK-NEXT: tail call
+; CHECK-NEXT: ret
----------------
`tail call _Z1fi` would be clearer.

================
Comment at: test/Transforms/MergeFunc/merge-block-address.ll:99
@@ +98,2 @@
+
+!0 = !{!"clang version 3.8.0 (trunk 243996) (llvm/trunk 244156)"}
----------------
Same on metadata and comments above.


http://reviews.llvm.org/D12376