This is an archive of the discontinued LLVM Phabricator instance.

[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

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.Jul 25 2019, 6:17 AM
test/MC/Mips/sym-sc.s
51

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.Aug 6 2019, 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.Aug 6 2019, 9:05 AM
mbrkusanin updated this revision to Diff 213829.Aug 7 2019, 3:00 AM
  • Removed unused function declarations.
atanasyan accepted this revision.Aug 7 2019, 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

clang-format

365

ditto

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