This is an archive of the discontinued LLVM Phabricator instance.

[mips][FastISel] Remove hidden mips-fast-isel option.
ClosedPublic

Authored by vkalintiris on Jul 29 2015, 2:36 PM.

Details

Summary

This hidden option would disable code generation through FastISel by
default. It was removed from the available options and from the
Fast-ISel tests that required it in order to run the tests.

Diff Detail

Repository
rL LLVM

Event Timeline

vkalintiris retitled this revision from to [mips][FastISel] Remove hidden mips-fast-isel option..
vkalintiris updated this object.
vkalintiris added a reviewer: dsanders.
vkalintiris added a subscriber: llvm-commits.

Hi, some inline comments :)

-eric

test/CodeGen/Mips/Fast-ISel/bswap1.ll
2–5 ↗(On Diff #30948)

You shouldn't need -fast-isel=true on these lines any more.

test/CodeGen/Mips/Fast-ISel/div1.ll
2–4 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/fastcc-miss.ll
2 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/logopm.ll
1–2 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/memtest1.ll
2–5 ↗(On Diff #30948)

Again.

test/CodeGen/Mips/Fast-ISel/mul1.ll
2–4 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/rem1.ll
2–4 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/sel1.ll
2 ↗(On Diff #30948)

Ditto.

test/CodeGen/Mips/Fast-ISel/shift.ll
1 ↗(On Diff #30948)

Why -O1 here?

test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
2 ↗(On Diff #30948)

The typical thing is to just use llc -march here, but I gather optimization makes the testcase go away?

7 ↗(On Diff #30948)

Go ahead and clean this up separately.

vkalintiris marked 8 inline comments as done.

Removed needless instances of the -fast-isel option from the FastISel tests.

Hi Eric,

Thanks for your comments. I replaced -mips-fast-isel with sed and forgot to do the same for -fast-isel. It should be fine now.

Thanks,
Vasileios

test/CodeGen/Mips/Fast-ISel/sel1.ll
2 ↗(On Diff #30948)

I believe that this one is required because of the -O2 level. Otherwise, I get the following assert error:

Assertion `(!EnableFastISelAbort || TM.Options.EnableFastISel) && "-fast-isel-abort > 0 requires -fast-isel"

Would you like me to change the level and whatever CHECK-lines are needed in this review? There are several things that we have to clean-up in FastISel and it's my intention to do that after the upcoming release.

test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
2 ↗(On Diff #30948)

That's right. However, we need the function's attributes too. This fine balance of options forces the OptLevelChanger to change the function's opt-level from -O2 to -O0. With -debug-only=isel, I get the following output:

...
Changing optimization level for Function main
        Before: -O2 ; After: -O0             
....

I had to try by trial and error in order to get it right, after reading about this behaviour from lib/CodeGen/SelectionDag/SelectionDagISel.cpp and the comments at test/CodeGen/X86/dag-optnone.ll

dsanders accepted this revision.Jul 30 2015, 4:07 AM
dsanders edited edge metadata.

It will LGTM with the attribute issue resolved. It seems excessive for this small detail to block merging to the release branch though so I don't mind if you commit and resolve that bit in a follow up.

test/CodeGen/Mips/Fast-ISel/sel1.ll
2 ↗(On Diff #30968)

I agree that this one is necessary since Fast ISel isn't the default for -O2

test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
2 ↗(On Diff #30968)

However, we need the function's attributes too.

I don't entirely follow why we need the attributes. FastISel off-by-default and on-by-default but explicitly disabled should have the same effect.
Also, the 'Changing optimization level' message you mentioned looks like it's just applying the effect of the -O0 command line option.

There's some dubious looking code in OptLevelChanger's constructor that enables FastISel without consulting the command-line option like LLVMTargetMachine.cpp's addPassesToGenerateCode does. Are we sure that -fast-isel=false works?

This revision is now accepted and ready to land.Jul 30 2015, 4:07 AM

Hi Daniel,

I tried to take a quick look into the issue but I can't figure out exactly what's going on. I will commit this so that we can merge it to the release branch. I completely agree with you that in principle -fast-isel=false shouldn't produce any different output with/without this patch. However, I'll look it more thoroughly over the weekend by tracing the execution of llc with and without FastISel enabled.

Thanks,
V. Kalintiris

This revision was automatically updated to reflect the committed changes.