Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rL LLVM

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: https://reviews.llvm.org/differential/diff/211116/

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
test/MC/Mips/sym-sc.s
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.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
348 ↗(On Diff #213829)

clang-format

365 ↗(On Diff #213829)

ditto

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.