[PATCH] D144682: [MergeFuncs] Compare load instruction metadata

[X via Phabricator via llvm-commits]

dingxiangfei2009 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
----------------
nikic wrote:
> This comment should be dropped, as it's no longer range specific.
Comment dropped


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:180
     ConstantInt *RLow = mdconst::extract<ConstantInt>(R->getOperand(I));
+    if (LLow == RLow)
+      continue;
----------------
nikic wrote:
> 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.
I have added a `TODO` with some explanation.


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


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:199
+  if (!R)
+    return 1;
+
----------------
nikic wrote:
> 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.)
Checks are removed


================
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 *>>();
----------------
nikic wrote:
> 
Applied


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


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

https://reviews.llvm.org/D144682