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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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 |
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) |
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. 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? |
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