Page MenuHomePhabricator

[mips][micromips] Add pattern for JmpLink to TargetExternalSymbol

Authored by abeserminji on Oct 17 2018, 4:06 AM.



When matching MipsISD::JmpLink t9, TargetExternalSymbol:i32'memset'...
wrong JALR16_MM is selected. This patch adds missing pattern for JmpLink,
so that JAL instruction is selected.

Diff Detail


Event Timeline

abeserminji created this revision.Oct 17 2018, 4:06 AM
This revision is now accepted and ready to land.Oct 21 2018, 7:50 AM
sdardis added inline comments.Oct 22 2018, 1:06 PM
1454 ↗(On Diff #169969)

This is in the wrong place. It should proceed the tail call patterns on line 1274.

1 ↗(On Diff #169969)

This test can be made better by declaring an external function f rather than memset as the memset intrinsic could be expanded inline by the MIPS backend in the future in any number of ways, e.g. lwm/swm in a loop for O3.

Another thing is that the test can be quicker by stopping after the isel expand pseudos pass and checking for the presence of JAL_MM, rather than assemblying an object then disassembling it. Another approach would be to extend test/CodeGen/Mips/llvm-ir/isel.ll for this case.

abeserminji added inline comments.Oct 24 2018, 3:04 AM
1 ↗(On Diff #169969)

Hello Simon, good to hear from you

I tried the solution you proposed before submitting this one, but it didn't work.
If I declare an external function, I don't get a call with JAL.
Then I found this very old post:
So I guess if it still stands, it explains why I couldn't use external function for this test.

And for the second part of the comment, I will make these changes with new update.

abeserminji marked an inline comment as done.

Resolved some comments.

sdardis accepted this revision.Oct 29 2018, 12:48 PM

LGTM. Just remember to upload the patches with context.

This revision was automatically updated to reflect the committed changes.