Page MenuHomePhabricator

[CSInfo][MIPS] Describe parameter value loaded by ADDiu instruction
ClosedPublic

Authored by ntesic on Apr 14 2020, 6:43 AM.

Details

Summary

Describe parameter's value loaded by MIPS ADDiu instruction.
When parameter's value is loaded into a register by mips ADDiu/DADDiu instruction,
it could be described correctly and emitted as DW_AT_GNU_call_site_value.

Test:
-dbg-call-site-param-addiu.mir, dbg-call-site-param-addiu-64bit.mir - confirm correct DW_AT_GNU_call_site_value of call site parameter DIE for parameter which value is loaded by ADDiu/DADDiu instruction.

Diff Detail

Event Timeline

ntesic created this revision.Apr 14 2020, 6:43 AM
dstenb added inline comments.Apr 14 2020, 9:49 AM
llvm/lib/Target/Mips/MipsInstrInfo.cpp
849

Nit: >80 cols

There are also some other minor indentation things here, so it might be good to run this patch through git-clang-format.

874

The AArch64 implementation (and the other target implementations) now check if the first operand really is a register: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp#L6610.

For AArch64, that seems to have been done due to encountering INLINEASM that defined the parameter register.

Should the same check be added here?

llvm/test/DebugInfo/MIR/Mips/dbg-call-site-param-addiu-64bit.mir
17

Personal opinion, but I prefer having the RUN lines at the top of the file, as I think that makes it easier to get an overview of what is tested, without having to scan the file.

ntesic marked 2 inline comments as done.Apr 23 2020, 5:24 AM
ntesic added inline comments.
llvm/lib/Target/Mips/MipsInstrInfo.cpp
874

I agree, we should consider that case for Mips also. Thanks.

llvm/test/DebugInfo/MIR/Mips/dbg-call-site-param-addiu-64bit.mir
17

Definitely, I can use that principle. Thanks.

ntesic updated this revision to Diff 259541.Apr 23 2020, 5:25 AM

Address comments.

vsk added a comment.Thu, May 7, 11:44 AM

This looks reasonable to me, but I'll defer to someone with more Mips background to give an official lgtm.

Herald added a project: Restricted Project. · View Herald TranscriptThu, May 7, 11:44 AM
atanasyan accepted this revision.Thu, May 7, 11:10 PM

Looks good overall, just a few nits inline.

llvm/lib/Target/Mips/MipsInstrInfo.cpp
850

You can join declaration and initialization.

869

You can join declaration and initialization.

883

The second "value" in this comment is redundant.

885

I prefer to reverse this condition something like this. I think, in this case it's more clear what we need from the operands.

if (Dop.isReg() && (Sop1.isReg() || Sop1.isFI()) && Sop2.isImm())
  return RegImmPair{Sop1.getReg(), Sop2.getImm()};

return None;
This revision is now accepted and ready to land.Thu, May 7, 11:10 PM
dstenb added inline comments.Fri, May 8, 1:44 AM
llvm/lib/Target/Mips/MipsInstrInfo.cpp
885

Oh, and the getReg() invocation will trigger an assert/produce incorrect data in the Sop1.isFI() case! What is the expected behavior if the operand is a frame-index?

djtodoro added inline comments.Fri, May 8, 6:43 AM
llvm/test/DebugInfo/MIR/Mips/dbg-call-site-param-addiu.mir
100

The same tip for trimming out the test: Use only one DILocation per scope.

ntesic marked an inline comment as done.Tue, May 12, 6:46 AM
ntesic added inline comments.
llvm/lib/Target/Mips/MipsInstrInfo.cpp
885

@atanasyan Thanks for suggestions.
@dstenb This was leftover from an old implementation. Case of ADDiu with frame index operand, should be considered separately. We can omit it for now and mark it with TODO.

ntesic updated this revision to Diff 265689.Fri, May 22, 3:01 AM

Address comments.
Rebase.

ntesic marked 6 inline comments as done.Fri, May 22, 3:02 AM
djtodoro accepted this revision.EditedWed, May 27, 4:20 AM

Thanks, lgtm. Please wait for others (for another day at least) to give their final comments.

In addition, I feel like mentioning the utility that could be useful for finding (target dependent) candidates for the DW_AT_call_value description: https://github.com/djolertrk/call-site-params-stats.
This could be useful when someone starts extending the support to other architectures as well.

This revision was automatically updated to reflect the committed changes.