[PATCH] D48331: [WIP][DebugInfo][InstCombine] Preserve DI after merging zext instructions

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1094
+            if (DIExp) {
+              return DIExpression::createFragmentExpression(
+                         DIExp, DestBitSize - SrcBitsKept,
----------------
gramanas wrote:
> aprantl wrote:
> > gramanas wrote:
> > > aprantl wrote:
> > > > gramanas wrote:
> > > > > vsk wrote:
> > > > > > You mentioned offline that this causes a verifier failure. If we can't use fragments, consider using a simpler approach for scalars: using an empty expression. It won't work for vectors, but in the scalar case, the high bits should already be cleared. We can relax the check in https://reviews.llvm.org/D48408 to allow this.
> > > > > If I use an empty DIExpression it doesn't change the fact that the dbg.value call points to a DILocalVariable that after instcombine has a different DIBasicType than what it used to.
> > > > > 
> > > > > Basiacly the dbg.value call says that the value was changed to an i32 but the it's type is still i8 in the debug info. If there is a way to explain this to the debugger using DIExpressions, I can't find it.
> > > > (Not sure if that is your question) We don't have any means to express that the SSA value used by a dbg.value instrinsic is larger (e.g., because of s/zext) than the size of the fragment being described. 
> > > > 
> > > > For example, if the size of the variable is 8 bits and the value is 32 bits then implicitly the lower 8 bits are used. You could make this explicit by masking out everything but the lower out bits in the DIExpression (`DW_OP_constu 0xff DW_OP_and`) but this is redundant so we don't do it.
> > > > 
> > > > Why would you like to express this?
> > > I thought the fact that a 32 bit variable has DIBasicType of size 8 is wrong. But modifying the DI would interfere with the way the front-end described the program at the first place, thus I was thinking of a way to describe it with a DIExpression. Since the correct value is being displayed implicitly, I guess it's not a problem.
> > When you say "32 bit variable" are you referring to the source variable or the SSA value that is used by the dbg.value intrinsic?
> When running `opt -debugify -instcombine` to the scalar test below, the `zext` is replaced with a wider `mul` (i32 from i8) and the resulting dbg.value from this patch points to a DILocalVariable that has DIBasicType of size 8, while its an i32 value. 
> 
> I think I mean the SSA value.
It looks like it's OK for a integer dbg.value operand to be wider than the type of its associated DILocalVariable. Adrian's second-to-last comment states that the debugger will handle this by displaying the lower bits.


Repository:
  rL LLVM

https://reviews.llvm.org/D48331