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

[Davide Italiano via Phabricator via llvm-commits]

davide marked 3 inline comments as done.
davide added inline comments.


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


================
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);
----------------
dblaikie wrote:
> 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
Indeed.


================
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));
----------------
dblaikie wrote:
> 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;
> ```
> 
Yeah, thanks for the suggestion.


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

https://reviews.llvm.org/D78147