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

[Robert Lougher via llvm-commits]

rob.lougher added a comment.

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.

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).

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.

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

Thanks,
Rob.


https://reviews.llvm.org/D26256