Page MenuHomePhabricator

[Mips] Instruction `sc` now accepts symbol as an argument

Authored by mbrkusanin on Jul 5 2019, 8:52 AM.



Function MipsAsmParser::expandMemInst() did not properly handle instruction sc
with a symbol as an argument because first argument would be counted twice.
We add additional checks and handle this case separately.

Diff Detail


Event Timeline

mbrkusanin created this revision.Jul 5 2019, 8:52 AM

I cannot invent any negative test case / example, but I'm concerned a bit that we completely drop information about out-register. The sc instruction has output result but we do not track that fact in td file just in our mind.

Alternative solution that does not remove out-register would look something like this:

mbrkusanin edited the summary of this revision. (Show Details)
  • Decided to apply diff of the alternative solution because the patch could not be downloaded with just the provided diff.
atanasyan added inline comments.Thu, Jul 25, 6:17 AM
51 ↗(On Diff #211524)

Could you modify this test case or add a new one to check handling symbol and offset? Like this sc $12, some_random_symbol_name + 8.

mbrkusanin updated this revision to Diff 213629.Tue, Aug 6, 9:05 AM

Symbol with offset was not handled properly on MipsR6. R_MIPS_LO16 (MipsMCExpr::MEK_LO) would write offset into lower 16 bits, where as for R6 version offset is in bits 7-15. Initially I made new type of MCFixup which would only use those bits, but then there was a lot of additional code needed for handling all cases where MCFixups were used. Now we just add ADDIU for R6 and use R_MIPS_LO16 on that instruction which is the same way that gcc works.

mbrkusanin marked an inline comment as done.Tue, Aug 6, 9:05 AM
mbrkusanin updated this revision to Diff 213829.Wed, Aug 7, 3:00 AM
  • Removed unused function declarations.
atanasyan accepted this revision.Wed, Aug 7, 4:08 AM

LGTM with a few nits. I will fix them by myself before committing the patch.

348 ↗(On Diff #213829)


365 ↗(On Diff #213829)


This revision is now accepted and ready to land.Wed, Aug 7, 4:08 AM
This revision was automatically updated to reflect the committed changes.