This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Add FPU Delay Slot for MIPS1/2/3
ClosedPublic

Authored by overdrivenpotato on Dec 5 2021, 2:24 PM.

Details

Summary

MIPS I, II, and III have delay slots for floating point comparisons and floating point register transfers (mtc1, mfc1). Currently, these are not taken into account and thus broken code may be generated on these targets. This patch inserts nops as necessary, while attempting to leave the current instruction if it is safe to stay.

The tests in this patch were updated by @sajattack

Sources:

https://gcc.gnu.org/legacy-ml/gcc-patches/2003-05/msg01601.html
https://gcc.gnu.org/legacy-ml/gcc-patches/2003-05/msg01605.html
https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/gcc/config/mips/mips.h#L1282
"See MIPS Run" Textbook: https://user-images.githubusercontent.com/1562711/144544253-de268f41-d808-4076-a500-e2cdba7f4294.png
MIPS R4000 Manual: https://user-images.githubusercontent.com/1562711/144544877-8896b4e9-3127-45a6-99d7-48b53a9b35e6.png

Diff Detail

Event Timeline

overdrivenpotato requested review of this revision.Dec 5 2021, 2:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2021, 2:24 PM
atanasyan accepted this revision.Dec 5 2021, 2:57 PM

LGTM with some nits.

llvm/lib/Target/Mips/MipsInstrInfo.cpp
576

Remove curly brackets here.

590

You can use std::tie here and provide meaningful names instead of Rw.first and Rw.second.

593

Remove curly brackets here.

This revision is now accepted and ready to land.Dec 5 2021, 2:57 PM

Fixed requested nits

djtodoro added inline comments.Dec 6 2021, 2:06 AM
llvm/lib/Target/Mips/MipsBranchExpansion.cpp
866

This code is being copied from handleForbiddenSlot() (most of it, actually). Can we factor/move the common code into a function/predicate?

llvm/lib/Target/Mips/MipsInstrInfo.cpp
588

an early exit here?

if (!Op.isReg())
  continue;
overdrivenpotato marked 3 inline comments as done.
overdrivenpotato marked 2 inline comments as done.

Fixed remaining nits and also added support for double precision FPU delay slots.

djtodoro accepted this revision.Dec 7 2021, 1:18 AM

lgtm, thanks

Please let me know if you need help with committing this (If you don't have the permission, I can do it for you).

Yes please, I don’t have commit access. Can you commit as Marko Mijalkovic <marko.mijalkovic97@gmail.com>?

Yes please, I don’t have commit access. Can you commit as Marko Mijalkovic <marko.mijalkovic97@gmail.com>?

We usually add "Patch by " at the end of the commit message.

This revision was automatically updated to reflect the committed changes.
jrtc27 added a comment.Dec 7 2021, 6:23 AM

Yes please, I don’t have commit access. Can you commit as Marko Mijalkovic <marko.mijalkovic97@gmail.com>?

We usually add "Patch by " at the end of the commit message.

That's not the official way to do it since we migrated to Git; see https://llvm.org/docs/DeveloperPolicy.html#attribution-of-changes which says to set the author field to the actual author.

Yes please, I don’t have commit access. Can you commit as Marko Mijalkovic <marko.mijalkovic97@gmail.com>?

We usually add "Patch by " at the end of the commit message.

That's not the official way to do it since we migrated to Git; see https://llvm.org/docs/DeveloperPolicy.html#attribution-of-changes which says to set the author field to the actual author.

Thanks for pointing that out!