[PATCH] D78147: [LICM] Try to merge debug locations when sinking

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Do you have any detailed data to refute the claim in the original comment "given that the inserted loads/stores have little relation to the original loads/stores".

Is this loop over the "original loads and stores"?

& maybe some comments in the test case describing what got merged and why the merged result is the desired one?



================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:81-82
+    std::vector<const DILocation *> &Locs) {
+  if (Locs.size() < 2)
+    return nullptr;
+  auto *Merged = Locs[0];
----------------
If there's only 1, probably should return that rather than nullptr?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:84
+  auto *Merged = Locs[0];
+  for (auto It = std::next(Locs.begin()); It != Locs.end(); ++It) {
+    Merged = getMergedLocation(Merged, *It);
----------------
REtrieve the end iterator once, rather than N times - as per the LLVM style guide? https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2066-2070
+  // Look at all the loop uses, and try to merge their locations.
+  std::vector<const DILocation *> LoopUsesLocs;
+  for (auto U : LoopUses)
+    LoopUsesLocs.push_back(U->getDebugLoc().get());
+  auto DL = DebugLoc(DILocation::getMergedLocations(LoopUsesLocs));
----------------
Bit unweildy if you have to write a loop and stash all the DILocations in a buffer to start with, maybe a templated API would be in order:

```
auto DL = DebugLoc(DILocation::getMergedLocation(LoopUses, [&](Instruction *I) {
  return I->getDebugLoc().get();
});
```

(whether or not this will generalize to the next case of N-way merging, anyone's guess (eg: the next case might need to filter some things out of the container - then this API would be uinadequate because it doesn't provide a means of skipping elements), but seems tidy enough for this case)

Or, I guess - this API could take a generic range (general templated Range type) & use mapped range... 

```
auto DL = DebugLoc(DILocation::getMergedLocation(map_range(LoopUses, [&](Instruction *I) {
  return I->getDebugLoc().get();
});
```

In that case it might be nice to modify the implementation to tolerate non-random-access ranges:

```
auto I = Range.begin();
auto E = Range.end();
if (I == E)
  return nullptr;
const DILocation *Result = *I;
++I;
for (; I != E; ++I) {
  Merged = getMergedLocation(Merged, *I);
  if (!Merged)
    break;
}
return Merged;
```



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

https://reviews.llvm.org/D78147