[llvm] r185993 - Un-break the buildbot by tweaking the indirection flag.

[David Blaikie]

On Fri, Jul 12, 2013 at 3:34 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Jul 12, 2013, at 3:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Wed, Jul 10, 2013 at 5:53 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>> On Jul 10, 2013, at 2:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> On Wed, Jul 10, 2013 at 2:56 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>
>>>>> On Jul 10, 2013, at 2:51 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>
>>>>>> So we're on the same page: isIndirect implies that the IR type is a
>>>>>> pointer. That was my intention, at least - is that clear/agreed?
>>>>>
>>>>> That the IR type is a pointer of foo, but the type of the DIVariable is foo. (As in your example with nontrivial by-value arguments).
>>>>
>>>> Yep.
>>>
>>> Hi David,
>>>
>>> I’ve got it working now. There is one thing I’d like to double-check with you before I commit my changes, though.
>>>
>>> Can you elaborate on this change you made in r184387?
>>>
>>>> @@ -384,13 +385,17 @@ void CompileUnit::addRegisterOffset(DIE *TheDie, unsigned
>>>> /// addAddress - Add an address attribute to a die based on the location
>>>> /// provided.
>>>> void CompileUnit::addAddress(DIE *Die, unsigned Attribute,
>>>> -                             const MachineLocation &Location) {
>>>> +                             const MachineLocation &Location, bool Indirect) {
>>>>   DIEBlock *Block = new (DIEValueAllocator) DIEBlock();
>>>>
>>>> -  if (Location.isReg())
>>>> +  if (Location.isReg() && !Indirect)
>>>>     addRegisterOp(Block, Location.getReg());
>>>> -  else
>>>> +  else {
>>>>     addRegisterOffset(Block, Location.getReg(), Location.getOffset());
>>>> +    if (Indirect && !Location.isReg()) {
>>>> +      addUInt(Block, 0, dwarf::DW_FORM_data1, dwarf::DW_OP_deref);
>>>> +    }
>>>> +  }
>>>>
>>>
>>> The commit message does not really mention it.
>>> Specifically, can there be a situation where MachineLocation::isIndirect() and DIVariable::isIndirect() disagree?
>>
>> I believe they're orthogonal. In fact I'm pretty sure in the case I
>> needed it they're decidedly would /not/ agree (the parameter was
>> stored directly in a register (not indirect) - but the ABI-level
>> parameter was a pointer (due to a non-trivial by-value parameter) to
>> the object in question. So we needed to record in the IR debug info
>> metadata that the parameter wasn't at the location given by the
>> dbg.value - but indirected through the pointer there.)
>>
>> Make sense?
>
> Your explanation makes sense, thanks! Do we actually have a testcase for that situation?

It would've been committed along with the change...
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/parameters.ll?view=markup&pathrev=184368

Which reminds me why supporting a zero offset in a MachineLocation was
insufficient for my needs. It wouldn't've solved the case where the
parameter was passed on the stack (once you run out of registers to
use) - which would've already have been reg+offset to get to the
pointer, so we needed /another/ layer of indirection on top of that.
It would be insufficient to use the indirection in the
MachineLocation.

We're probably going to need a far more general/powerful way to
describe locations through optimizations at some point - so I'm open
to suggestions that might take us in that direction.

>
>>
>>> My patch essentially would revert this change, because we are now piping the IsIndirect flag all the way down to the MachineLocation, but I want to make sure that I’m not missing something.
>>
>> I haven't really looked at your patch deeply enough to fully
>> understand this. There was a reason that I didn't do this in my
>> original change even though I wrote the patch that could do this (for
>> dbg_value MI intrinsics, not for the MachineLocation itself) but
>> didn't end up needing i- though I don't fully recall why it wasn't
>> suitable for my needs in the end.
>
> In the light of this it seems that we need to update the documentation:
>
> DIVariable::isIndirect() - Indicates that the variable is stored indirect (such as a VLA).
> MachineLocation::isIndirect() - Indicates a layer of indirection introduced by the ABI (such as a byval struct).

I haven't thought about this enough yet to understand the "byval
struct" example & how it relates. Might be easier to discuss on IRC or
something (but if you could provide more detail here I'll do my best)

>
> This also means that it is not correct to ket MachineLocation inherit the DIVariable::isIndirect flag during lowering. That is -- unless it is impossible to have two layers of indirection...

See explanation above - it is quite possible to have two layers. Try a
non-trivial by-val parameter that is not passed in a register.

>
> I’m going to work on that.
>
> -- adrian