Page MenuHomePhabricator

[DebugInfo] Make describeLoadedValue() reg aware
ClosedPublic

Authored by dstenb on Tue, Nov 19, 4:09 AM.

Details

Summary

Currently the describeLoadedValue() hook is assumed to describe the
value of the instruction's first explicit define. The hook will not be
called for instructions with more than one explicit define.

This commit adds a register parameter to the describeLoadedValue() hook,
and invokes the hook for all registers in the worklist.

This will allow us to for example describe instructions which produce
more than two parameters' values; e.g. Hexagon's various combine
instructions.

This also fixes situations in our downstream target where we may pass
smaller parameters in the high part of a register. If such a parameter's
value is produced by a larger copy instruction, we can't describe the
call site value using the super-register, and we instead need to know
which sub-register that should be used.

This also allows us to handle cases like this:

$ebx = [...]
$rdi = MOVSX64rr32 $ebx
$esi = MOV32rr $edi
CALL64pcrel32 @call

The hook will first be invoked for the MOV32rr instruction, which will
say that @call's second parameter (passed in $esi) is described by $edi.
As $edi is not preserved it will be added to the worklist. When we get
to the MOVSX64rr32 instruction, we need to describe two values; the
sign-extended value of $ebx -> $rdi for the first parameter, and $ebx ->
$edi for the second parameter, which is now possible.

This commit modifies the dbgcall-site-lea-interpretation.mir test case.
In the test case, the values of some 32-bit parameters were produced
with LEA64r. Perhaps we can in general cases handle such by emitting
expressions that AND out the lower 32-bits, but I have not been able to
land in a case where a LEA64r is used for a 32-bit parameter instead of
LEA64_32 from C code.

I have not found a case where it would be useful to describe parameters
using implicit defines, so in this patch the hook is still only invoked
for explicit defines of forwarding registers.

Diff Detail

Event Timeline

dstenb created this revision.Tue, Nov 19, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 19, 4:10 AM
dstenb marked an inline comment as done.Tue, Nov 19, 4:27 AM
dstenb added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1174

A bit related to the above comment, when working on this patch I encountered a case where an incorrect call site value is emitted when describing a 32-bit value on x86-64: https://bugs.llvm.org/show_bug.cgi?id=43343#c8.

vsk added inline comments.Tue, Nov 19, 11:24 AM
llvm/lib/CodeGen/TargetInstrInfo.cpp
1147

I think this should either be TRI->isSuperRegister(...), or the previous Reg == DestReg condition should be deleted. My preference would be to keep the cases separate, as I find that clearer.

1147

Why is it safe to assume the copy would be zero-extending here?

1154

Is it possible for the source & destination subregister indices to be different, in a copy? E.g.: if there were an instruction which copied the high 32 bits of %r8 into the low 32 bits of %r9. If so, maybe subregister copies would be safer to handle within specializations of describeLoadedValue.

If not, or we just don't know of any such cases, there should be a clear note about this assumption added to the isCopyInstr documentation.

1161

Ditto, not sure why 'isSuperRegister' isn't enough here.

1161

Ditto, why is it safe to assume this is a zero-extending add? E.g. I think 'sxta' on ARM works differently.

1174

Thanks for catching this!

llvm/lib/Target/X86/X86InstrInfo.cpp
7601

Might be cleaner to stash the LLVMContext & in a helper local var.

7602

Can this just be a helper method in DIExpression? It's possible that replaceAllDbgUsesWith could use this.

dstenb marked an inline comment as done.Tue, Nov 19, 2:37 PM
dstenb added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1161

Looking around at a few target, it seemed like in such cases the whole extended super-register would be the explicit define, but I guess there's nothing that says that that must be the case.

I'll look into adding a Register parameter to isAddImmediate(), and let that handle whether or not super-registers can be described (and if so, if the value should be sign-extended).

vsk added a comment.Tue, Nov 19, 6:08 PM

Does this reduce/remove the need for this piece of logic in AArch64's ISel lowering?

// Call site info is used for function's parameter entry value
// tracking. For now we track only simple cases when parameter
// is transferred through whole register.
CSInfo.erase(std::remove_if(CSInfo.begin(), CSInfo.end(),
                            [&VA](MachineFunction::ArgRegPair ArgReg) {
                              return ArgReg.Reg == VA.getLocReg();
                            }),
             CSInfo.end());

Does this reduce/remove the need for this piece of logic in AArch64's ISel lowering?

//Call site info is used for function's parameter entry value
//tracking. For now we track only simple cases when parameter
// is transferred through whole register.
 CSInfo.erase(std::remove_if(CSInfo.begin(), CSInfo.end(),
        [&VA](MachineFunction::ArgRegPair ArgReg) {
    return ArgReg.Reg == VA.getLocReg();
}),
CSInfo.end());

I think so.

llvm/lib/Target/X86/X86InstrInfo.cpp
7602

+1

There are cases (e.g. salvageDebugInfo()) where this can be useful.

dstenb marked an inline comment as done.Wed, Nov 20, 2:23 PM
dstenb added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1147

I looked around at some targets, but I guess there's no contract that says that this is the case.

Walking back a bit of what I have done in this patch, maybe the TargetInstrInfo implementation should not attempt to handle such cases, and then we'll leave it up to each target to override the hook for copy instructions that may zero-extend?

If so, would it be fine to, at least as a start now that we work towards enabling call site information by default, add an assert that Reg is not a super-register of DestReg?

So something like this:

if (Reg == DestReg)
  return ParamLoadedValue(*Source, Expr);

assert(!TRI->isSuperRegister(DestReg, Reg) && "[...]");

[...]

return None;

I have not had the time to look into your concern below about sub-registers, but maybe the same should be done for such cases also.

vsk added inline comments.Wed, Nov 20, 3:52 PM
llvm/lib/CodeGen/TargetInstrInfo.cpp
1147

Yeah, I think that's a good idea, ditto for the sub-register case. Adding asserts in the generic code (here) is doubly-nice as it makes it easier for target maintainers to find what's missing.

1161

I see. One alternative might be to document+assert that the generic code cannot handle the described reg being a super-reg of the add destination (& to just defer this work to target-specific describeLoadedValue hooks). Not sure whether that'd be better or much different, just wanted to list the option for completeness.

IMO, if there is no specifications for recognizing the instructions that may zero-extend, the individual per-target handling seems good to me.

dstenb updated this revision to Diff 231254.Wed, Nov 27, 7:19 AM

Update the patch. Sorry for the delay!

This moves the sub- and super-register handling out of TargetInstrInfo's describeLoadedValue() implementation, and leaves it up to each target to override cases where that can occur. For copy instructions there are asserts, so that we can catch such cases.

To move the sub- and super-register handling out for add immediate instructions, this changes that hook so that it takes a register that it should describe. Since we may want to return a sub- or super-register, the return value is changed from a MachineOperand pointer to a plain Register. Since I'm not very familiar with AArch64, nor have a machine to test that, I did not dare to describe such cases.

I like the way of the implementation. Thanks!

llvm/include/llvm/CodeGen/TargetInstrInfo.h
74–82

Please add a comment here explaining the usage of the struct.

Maybe better RegImmPair ?

961–965

and a physical register and stores, and stores the

llvm/lib/CodeGen/TargetInstrInfo.cpp
1144

This looks good to me!

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6491

describeOrrLoadedValue ==> describeORRLoadedValue ?

llvm/lib/Target/X86/X86InstrInfo.cpp
7558

describeMovrrLoadedValue ==> describeMOVrrLoadedValue ?

7569

is a sub-register

7587

So, this covers all the cases of the super-registers except the X86::MOV32rr case?

djtodoro added inline comments.Thu, Nov 28, 1:27 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
961–965

and a physical register and stores ==> and a physical register, and stores the

dstenb updated this revision to Diff 231437.Thu, Nov 28, 7:41 AM

Address review comments.

dstenb marked 7 inline comments as done.Thu, Nov 28, 7:47 AM
dstenb added inline comments.
llvm/lib/Target/X86/X86InstrInfo.cpp
7587

Sorry, what is the question here? We only handle MOV32rr there (?).

djtodoro added inline comments.Thu, Nov 28, 11:21 PM
llvm/lib/Target/X86/X86InstrInfo.cpp
7587

Sorry for the confusion.. I have made a mistake. :)
I was going to say that the line covers only the MOV64rr case when the described register is a super-register of the destination register. I was thinking about adding a comment above the line, but it is obvious, so I guess we do not need the one.

vsk added inline comments.Thu, Dec 5, 4:57 PM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5337

Probably worth adding a comment here similar to the ones elsewhere, about cases where the add defines a super/sub register of the described reg not being supported yet.

llvm/lib/Target/X86/X86InstrInfo.cpp
7708

IIUC, at this point, Reg must be a sub-register of operand 0 or equal to it. Can operand 0 of a MOVSX64rr32 ever not be 64-bit, and if not, is this check necessary?

dstenb marked 2 inline comments as done.Fri, Dec 6, 7:01 AM
dstenb added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5337

Yes!

llvm/lib/Target/X86/X86InstrInfo.cpp
7708

The MachineVerifier complains if operand 0 is not a 64-bit register. That check is still necessary as it's there to detect if the described register is 32/64 bits. However, I don't understand why I did it that complicated instead of just using a simple (Reg == MI.getOperand(0).getReg) check.

I'll switch to (Reg == MI.getOperand(0).getReg()). I'll also add an assert that the described register otherwise is the 32 bit sub-register, so that we don't silently produce incorrect call site values for 8- and 16-bit sub-registers. AFAICT we currently can never land in a situation where we need to describe a 8- or 16-bit sub-register, but perhaps later on we will.

dstenb updated this revision to Diff 232552.Fri, Dec 6, 7:05 AM

Address review comments.

vsk accepted this revision.Fri, Dec 6, 7:41 AM

Thanks, lgtm.

This revision is now accepted and ready to land.Fri, Dec 6, 7:41 AM
dstenb added a comment.Fri, Dec 6, 9:47 AM

Thanks for the reviews!

Any more comments on this, @djtodoro?

djtodoro accepted this revision.Sun, Dec 8, 11:23 PM

LGTM as well! Thanks!

This revision was automatically updated to reflect the committed changes.