Page MenuHomePhabricator

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

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



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.

-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

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.


The AArch64 implementation (and the other target implementations) now check if the first operand really is a register:

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

Should the same check be added here?


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.

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


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.May 7 2020, 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 TranscriptMay 7 2020, 11:44 AM
atanasyan accepted this revision.May 7 2020, 11:10 PM

Looks good overall, just a few nits inline.


You can join declaration and initialization.


You can join declaration and initialization.


The second "value" in this comment is redundant.


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.May 7 2020, 11:10 PM
dstenb added inline comments.May 8 2020, 1:44 AM

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.May 8 2020, 6:43 AM

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

ntesic marked an inline comment as done.May 12 2020, 6:46 AM
ntesic added inline comments.

@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.May 22 2020, 3:01 AM

Address comments.

ntesic marked 6 inline comments as done.May 22 2020, 3:02 AM
djtodoro accepted this revision.EditedMay 27 2020, 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:
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.