Page MenuHomePhabricator

[mips][FastISel] Instantiate the MipsFastISel class only for targets that support FastISel.
ClosedPublic

Authored by vkalintiris on Sep 22 2016, 5:34 AM.

Details

Summary

Instead of instantiating the MipsFastISel class and checking if the
target is supported in the overriden methods, we should perform that
check before creating the class. This allows us to enable FastISel *only*
for targets that truly support it, ie. MIPS32 to MIPS32R5.

Diff Detail

Repository
rL LLVM

Event Timeline

vkalintiris retitled this revision from to [mips][FastISel] Instantiate the MipsFastISel class only for targets that support FastISel..
vkalintiris updated this object.
vkalintiris added a subscriber: llvm-commits.
vkalintiris removed a subscriber: sdardis.
sdardis requested changes to this revision.Sep 27 2016, 3:16 PM
sdardis edited edge metadata.

Comments inlined.

lib/Target/Mips/MipsISelLowering.cpp
464–465 ↗(On Diff #72157)

I find this comment strange given the block beneath. I think this should be:

// FastISel is supported only for [MIPS32,MIPS32R5] ISAs.
bool UseFastISel = TM.Options.EnableFastISel && Subtarget.hasMips32() && !Subtarget.hasMips32r6());
467–469 ↗(On Diff #72157)

With my comment above, I think this should check for inMicroMips(), with the comment reflecting that we don't support compressed instruction sets.

Aside, I've barely looked but is mips16e rejected at a higher level for FastISel?

This revision now requires changes to proceed.Sep 27 2016, 3:16 PM
vkalintiris edited edge metadata.

Cleanup FastISel support checks and remember to reject it on MIPS16 as well.

ehostunreach added inline comments.
lib/Target/Mips/MipsISelLowering.cpp
464–465 ↗(On Diff #72876)

That was intentional as a coding style, ie. set UseFastISel and use the checks that follow to unset it. I merged the ISA-checks together.

467–469 ↗(On Diff #72876)

I added a check for the MIPS16 mode and a test case as well.

sdardis accepted this revision.Oct 8 2016, 11:40 AM
sdardis edited edge metadata.

LTGM, with one comment nit, inlined.

lib/Target/Mips/MipsISelLowering.cpp
464–465 ↗(On Diff #72876)

My request here is that the comment on what ISAs we support doesn't mention MIPS32R6. So that comment should be:

// We support only the standard encoding [MIPS32,MIPS32R5] ISAs.

Otherwise the comments don't quite match the code. IMHO.

This revision is now accepted and ready to land.Oct 8 2016, 11:40 AM
This revision was automatically updated to reflect the committed changes.
vkalintiris added inline comments.Oct 18 2016, 6:18 AM
lib/Target/Mips/MipsISelLowering.cpp
464–465 ↗(On Diff #72876)

Sorry, I didn't understand you initial comment. I updated the review before committing the patch and replaced the MIPS32R6 bit with the MIPS32R5 ISA.