[PATCH] D26256: [InstCombine] Don't set debug location when folding through a phi node

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In https://reviews.llvm.org/D26256#601321, @rob.lougher wrote:

> Just a quick note to say I haven't forgotten about this.
>
> My understanding is if we had
>
> #1 v = cond ?
>  #2 stmt1: stmt2;
>
> Then if an instruction was commoned, we should still have at least one instruction left in the blocks for stmt1 and stmt2?  So debugging experience wouldn't be affected as we'll still stop at line 2.
>
> However, from Adrian's last comment, we're now moving away from a purely functional argument, to aesthetics of the code where we want something to say "yes, I've thought about debug info here", even if we simply do nothing.
>
> I'm not against doing this.  However, before I submit a revised patch I'd like to run a few things past people.
>
> First, where should a merge API live?  I can see two possible places, the DebugLoc or the DILocation class.  Get/SetDebugLoc uses DebugLoc, but in DebugLoc.h there's FIXMEs to avoid using it.


For symmetry with `DILocation *DILocation::cloneWithDiscriminator(unsigned Discriminator) const` DILocation seems like a natural place.

> Second, assuming we want to merge two locations, the function could be static or non-static:
> 
>   DILocation mergeDebugLoc(DILocation A, DILocation B)
>   DILocation merge(DILocation Other)
> 
> 
> If we need to merge more than two locations, this can be done iteratively, merging the next one onto the current merged result (hence a non-static function may be better).

I assume that both versions would be `const`? If so I think that the verb shouldn't just be merge as it implies that it modifies the object.

At the call site this would look like this:

  I.setDebugLoc(DILocation::mergeDebugLoc(LocA, LocB));
  I.setDebugLoc(LocA->merge(LocB));
  // More variants:
  I.setDebugLoc(DILocation::getMergedLocation(LocA, LocB));
  I.setDebugLoc(LocA->mergeWith(LocB));
  I.setDebugLoc(LocA->cloneMergedLocation(LocB));

Subjectively I like DILocation::getMergedLocation(LocA, LocB) best, but I'm open to other suggestions.

> As far as revising the patch to use the new API, there is an added complexity.  The optimisations are generalised, and the phi node may have any number of input values (not just two as in the example).  In this case we will need to iterate over all the incoming values, merging the debug locations together into a single location.  In some cases there is already a loop iterating over the values, in other cases a loop will need to be added.

Thanks for pointing this out I wasn't aware of that. Let me know if it grows impractical.

> My initial implementation of mergeDebugLoc() would simply return DebugLoc().

That's fine. Thanks for looking into this!

- adrian

> Thanks,
>  Rob.


https://reviews.llvm.org/D26256