[PATCH] D144682: Fix miscompilation from MergeFunctions-Inline-SROA-InstCombine optimization sequence

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:168
   // Range metadata is a sequence of numbers. Make sure they are the same
   // sequence.
   // TODO: Note that as this is metadata, it is possible to drop and/or merge
----------------
This comment should be dropped, as it's no longer range specific.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:180
     ConstantInt *RLow = mdconst::extract<ConstantInt>(R->getOperand(I));
+    if (LLow == RLow)
+      continue;
----------------
Add a TODO here that we need to compare other metadata contents. The current implementation will consider any metadata that doesn't contain numbers as equivalent, which isn't right. But I think it's fine to leave it for now, so we don't get carried away.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:193
+int FunctionComparator::cmpLoadInstMetadata(LoadInst const *L,
+                                            LoadInst const *R) const {
+  if (L == R)
----------------
I'd call this `cmpInstMetadata` and accept `Instruction` -- this code is not really load specific anymore.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:199
+  if (!R)
+    return 1;
+
----------------
These checks here don't make sense to me. At this point `L` and `R` should be non-null. (Could even accept them by reference.)


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:201
+
+  /// Compare metadata with empty nodes
+  /// These metadata affects the other optimization passes by making assertions
----------------
The "empty nodes" part shouldn't be there anymore.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:205
+  /// Values that carries different expectations should be considered different.
+  auto MDL = SmallVector<std::pair<unsigned, MDNode *>>();
+  auto MDR = SmallVector<std::pair<unsigned, MDNode *>>();
----------------



================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:219
+    if (KeyL == LLVMContext::MD_dbg)
+      continue;
+    if (int Res = cmpMetadata(ML, MR))
----------------
It would be better to use `getAllMetadataOtherThanDebugLoc()`.


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

https://reviews.llvm.org/D144682