Mips unconditionally uses fast-isel?

I was mucking around in FastISel, and was surprised to see the test
  llvm/test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
failed. This was surprising because it specifies -fast-isel=false.

Does the Mips code generator use fast-isel even when you ask it not to?
Thanks,
--paulr

This seems to be an all-targets bug. There's code in OptLevelChanger (lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp) that forces Fast ISel to be enabled for every -O0 function.

> I was mucking around in FastISel, and was surprised to see the test
> llvm/test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
> failed. This was surprising because it specifies -fast-isel=false.
>
> Does the Mips code generator use fast-isel even when you ask it not to?
> Thanks,
> --paulr

This seems to be an all-targets bug. There's code in OptLevelChanger
(lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp) that forces Fast ISel to
be enabled for every -O0 function.

Well.... the test specifies -O0 so even though the function is marked
'optnone' I'd expect OptLevelChanger to be a no-op, because it checks
whether the old and new OptLevel are the same. Which they should be.

If I use -debug-only=isel I see it is changing from -O2 to -O0, which
makes me think the "real" bug is that llc is ignoring -O0.
--paulr

> > I was mucking around in FastISel, and was surprised to see the test
> > llvm/test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
> > failed. This was surprising because it specifies -fast-isel=false.
> >
> > Does the Mips code generator use fast-isel even when you ask it not to?
> > Thanks,
> > --paulr
>
> This seems to be an all-targets bug. There's code in OptLevelChanger
> (lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp) that forces Fast ISel to
> be enabled for every -O0 function.

Well.... the test specifies -O0 so even though the function is marked
'optnone' I'd expect OptLevelChanger to be a no-op, because it checks
whether the old and new OptLevel are the same. Which they should be.

If I use -debug-only=isel I see it is changing from -O2 to -O0, which
makes me think the "real" bug is that llc is ignoring -O0.
--paulr

Looking further, I think it's a bit of both. Enabling Fast ISel when -fast-isel=false is given doesn't make sense but there are also five targets (Mips, BPF, Sparc, PPC, and AMDGPU) that use the SelectionDAGISel(TargetMachine &) constructor which leave the optimization level at the default. The other targets all pass on the optimization level using the two argument constructor.

> > > I was mucking around in FastISel, and was surprised to see the test
> > > llvm/test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
> > > failed. This was surprising because it specifies -fast-isel=false.
> > >
> > > Does the Mips code generator use fast-isel even when you ask it not
to?
> > > Thanks,
> > > --paulr
> >
> > This seems to be an all-targets bug. There's code in OptLevelChanger
> > (lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp) that forces Fast ISel
to
> > be enabled for every -O0 function.
>
> Well.... the test specifies -O0 so even though the function is marked
> 'optnone' I'd expect OptLevelChanger to be a no-op, because it checks
> whether the old and new OptLevel are the same. Which they should be.
>
> If I use -debug-only=isel I see it is changing from -O2 to -O0, which
> makes me think the "real" bug is that llc is ignoring -O0.
> --paulr

Looking further, I think it's a bit of both. Enabling Fast ISel when -
fast-isel=false is given doesn't make sense but there are also five
targets (Mips, BPF, Sparc, PPC, and AMDGPU) that use the
SelectionDAGISel(TargetMachine &) constructor which leave the optimization
level at the default. The other targets all pass on the optimization level
using the two argument constructor.

Okay... so Mips unconditionally has -O2 behavior from SelectionDAGISel,
regardless of the requested optimization level; is that really what you
wanted?
                        >
For this particular test, the -O0 is ignored and becomes -O2, which
defaults to -fast-isel=false, and then 'optnone' is actually where you
get the -O0 effect. Except that 'optnone' also turns ON fast-isel, which
you didn't notice because it didn't actually matter, until I came along
and started mucking around in FastISel.

I think if Mips wants -O0 to work in llc, then it really needs to be
passed in to SelectionDAGISel. Then take 'optnone' off the function in
this test, and it should all Just Work. And the test would actually be
exercising the non-fast-isel path, which it seems is what you wanted?

The other thing that might work, is having TargetMachine remember how
the fast-isel option got set, and make OptLevelChanger do the right
thing. But that seems like a hack to work around Mips not obeying the
specified optimization level, honestly.

Thanks,
--paulr

Okay... so Mips unconditionally has -O2 behavior from SelectionDAGISel,
regardless of the requested optimization level; is that really what you
wanted?

This code slightly pre-dates my joining LLVM so I don't know the thoughts behind it. I agree it looks wrong though.

For this particular test, the -O0 is ignored and becomes -O2, which
defaults to -fast-isel=false, and then 'optnone' is actually where you
get the -O0 effect. Except that 'optnone' also turns ON fast-isel, which
you didn't notice because it didn't actually matter, until I came along
and started mucking around in FastISel.

I think that sums it up. One observation to add: If I make SelectionDAGISel::runOnMachineFunction() unconditionally print the effective optimization level. Mips always picks -O2 as we've discussed but I can't get -O1 for X86 either (I get -O2 instead).

I think if Mips wants -O0 to work in llc, then it really needs to be
passed in to SelectionDAGISel. Then take 'optnone' off the function in
this test, and it should all Just Work. And the test would actually be
exercising the non-fast-isel path, which it seems is what you wanted?

I agree.

The other thing that might work, is having TargetMachine remember how
the fast-isel option got set, and make OptLevelChanger do the right
thing. But that seems like a hack to work around Mips not obeying the
specified optimization level, honestly.

I think we should do that as well. I don't think it's right that optnone enables Fast ISel even when it's been explicitly disabled. It should do the same checks as addPassesToGenerateCode() does.

> The other thing that might work, is having TargetMachine remember how
> the fast-isel option got set, and make OptLevelChanger do the right
> thing. But that seems like a hack to work around Mips not obeying the
> specified optimization level, honestly.

I think we should do that as well. I don't think it's right that optnone
enables Fast ISel even when it's been explicitly disabled. It should do
the same checks as addPassesToGenerateCode() does.

Hm? What you're asking for is that "-O2" and "-O2 -fast-isel=none" are
identical, unless you have an 'optnone' function. Do you really have a
use-case for controlling the codegen path for an 'optnone' function?
The whole point of 'optnone' is to avoid optimizations.
--paulr

No, that’s already true. -O2 doesn’t try to enable Fast ISel (unless the optnone attribute is given) so –fast-isel=false has no effect.

I’m saying that optnone means ‘use –O0 for this function’ and that optnone should

respect non-default values of the -fast-isel flag like –O0 does. This is the behaviour I’d expect:

Well, ‘optnone’ is already not identical to –O0, and given the nature of things, probably can’t be; but I am persuaded that it’s reasonable for it to honor the –fast-isel option as a debugging tactic. I’ll take an AI to make this happen.

Thanks,

–paulr

P.S. One nit, the “O0 + optnone” case should not have an asterisk, the FastISel flag is not manipulated if the opt level is already zero. Does not affect the strength of your argument, of course.

I’d have figured optnone was “no optimizations” not “use the entire O0 code path”?

At least that seemed to be the intent when you added it Paul?

-eric

Hi Daniel, Paul

Thanks for looking in to this. I think its great to fix the inconsistency here.

...

> From: Robinson, Paul [mailto:Paul_Robinson@playstation.sony.com]
> Sent: 17 November 2015 22:58
> To: Daniel Sanders; llvm-dev@lists.llvm.org <mailto:llvm-dev@lists.llvm.org>
> Subject: RE: Mips unconditionally uses fast-isel?
>
> > > The other thing that might work, is having TargetMachine remember how
> > > the fast-isel option got set, and make OptLevelChanger do the right
> > > thing. But that seems like a hack to work around Mips not obeying the
> > > specified optimization level, honestly.
> >
> > I think we should do that as well. I don't think it's right that optnone
> > enables Fast ISel even when it's been explicitly disabled. It should do
> > the same checks as addPassesToGenerateCode() does.
>
> Hm? What you're asking for is that "-O2" and "-O2 -fast-isel=none" are
> identical, unless you have an 'optnone' function. Do you really have a
> use-case for controlling the codegen path for an 'optnone' function?
> The whole point of 'optnone' is to avoid optimizations.
> --paulr
>

No, that's already true. -O2 doesn't try to enable Fast ISel (unless the optnone attribute is given) so –fast-isel=false has no effect.

I'm saying that optnone means 'use –O0 for this function' and that optnone should
respect non-default values of the -fast-isel flag like –O0 does. This is the behaviour I'd expect:

-fast-isel=false
-fast-isel=default
-fast-isel=true
-O0
SelectionDAG
FastISel
FastISel
-O0 + optnone attribute
SelectionDAG*
FastISel
FastISel
-O1 + optnone attribute
SelectionDAG*
FastISel
FastISel
-O2 + optnone attribute
SelectionDAG*
FastISel
FastISel
-O3 + optnone attribute
SelectionDAG*
FastISel
FastISel
-O1
SelectionDAG
SelectionDAG
FastISel
-O2
SelectionDAG
SelectionDAG
FastISel
-O3
SelectionDAG
SelectionDAG
FastISel
The cells marked with '*' differ from the current behavior.

This table is excellent. Any chance you can find a good place in code to put it, or even somewhere in the docs?

I did look to see if http://llvm.org/docs/WritingAnLLVMBackend.html#instruction-selector had a section on fast-isel but it surprisingly doesn’t. If it did, I think this table would have been good to put there to explain when a backend will get which selector.

Cheers,
Pete

The driving goal of ‘optnone’ is to have an easy way for programmers to get an “-O0 like” debugging experience for selected functions, without making them build everything with –O0.

To that end, we turn off as much optimization as we reasonably can, but in the context of a pipeline that is generally expecting optimizations to be enabled, in practice we can’t exactly match –O0 because “things break” if we turn off everything. Luckily, exactly matching –O0 isn’t a requirement.

Contemplating the “things break” situation, I am entirely willing to believe (it may have actually happened) that problems can occur when using FastISel in a pipeline that isn’t expecting it. So, for purposes of diagnosing these ‘optnone’ problems, it seems useful to be able to force ‘optnone’ not to use FastISel. This is a different motivation than Daniel expressed, which is a more principled idea coming from the “optnone should exactly match –O0” misconception; but the conclusion (that we should respect an explicit –fast-isel=false within ‘optnone’ functions) is the same.

–paulr

The driving goal of ‘optnone’ is to have an easy way for programmers to get an “-O0 like” debugging experience for selected functions, without making them build everything with –O0.

To that end, we turn off as much optimization as we reasonably can, but in the context of a pipeline that is generally expecting optimizations to be enabled, in practice we can’t exactly match –O0 because “things break” if we turn off everything. Luckily, exactly matching –O0 isn’t a requirement.

Contemplating the “things break” situation, I am entirely willing to believe (it may have actually happened) that problems can occur when using FastISel in a pipeline that isn’t expecting it. So, for purposes of diagnosing these ‘optnone’ problems, it seems useful to be able to force ‘optnone’ not to use FastISel. This is a different motivation than Daniel expressed, which is a more principled idea coming from the “optnone should exactly match –O0” misconception; but the conclusion (that we should respect an explicit –fast-isel=false within ‘optnone’ functions) is the same.

Works for me, should probably document it under some documentation for the function attribute.

Thanks!

-eric

It looks like the Mips target is not ignoring –O0 completely; only for ISel purposes. In the test I mentioned originally (Mips/emergency-spill-slot-near-fp.ll) I can remove –fast-isel=false and ‘optnone’ from the test, and it passes; but it still needs –O0, or the spill that it’s looking for doesn’t happen.

Once Mips pays attention to –O0 for ISel purposes, then it would need –fast-isel=false again, I think, because the test is not using FastISel now.

–paulr

Well, ‘optnone’ is already not identical to –O0, and given the nature of things, probably can’t be

It’s not important to the topic but I’m curious about this since, in instruction selection at least, they both set SelectionDAGISel::OptLevel to CodeGenOpt::None and MCCodeGenInfo::OptLevel by one means or another. Could you point me at a difference between the two? I assume it’s outside of instruction selection.

P.S. One nit, the “O0 + optnone” case should not have an asterisk, the FastISel flag is not manipulated if the opt level is already zero. Does not affect the strength of your argument, of course.

Bugs aside, you’re right. I made this table using the Mips target so it was switching from O2 to O0.

I’ll try to fix that everything-is-O2 bug today/tomorrow. It will probably be tomorrow due to other commitments.

This is a different motivation than Daniel expressed, which is a more principled idea coming from the “optnone should exactly match –O0” misconception

It doesn’t change anything but just for the record: While I do think that this should be true as far as possible, my opinion on -fast-isel=false is more rooted in the idea that emergency overrides shouldn’t be overridable by normal use. As an analogy, circuit breakers shouldn’t be overridable by a light switch.

Well, ‘optnone’ is already not identical to –O0, and given the nature of things, probably can’t be

It’s not important to the topic but I’m curious about this since, in instruction selection at least, they both set SelectionDAGISel::OptLevel to CodeGenOpt::None and MCCodeGenInfo::OptLevel by one means or another. Could you point me at a difference between the two? I assume it’s outside of instruction selection.

Without actually doing the research… There are interactions between DAG construction, DAG Combine in particular, possibly CodeGenPrepare, and ISel, which I don’t really understand. We tried to turn off all of DAG Combine, IIRC, but that totally didn’t work, so instead we turn off individual bits and pieces.

We made a distinct effort to find all the IR and MachineFunction passes that don’t run at O0 and turn them off individually (Chandler was particularly insistent that the pass manager should not be aware of ‘optnone’) and it’s always possible that we missed some; those cases should be properly considered bugs. And of course new passes get added from time to time, and typically people don’t think about ‘optnone’ when they’re doing that… again those are properly speaking bugs.

Regarding the “circuit-breaker” analogy, ‘optnone’ itself is a kind of user-visible circuit breaker, and making it not break the entire circuit could be considered a little strange. But as I said, it does seem like a useful debugging tactic for ‘optnone’ itself.

–paulr

Well, ‘optnone’ is already not identical to –O0, and given the nature of things, probably can’t be

It’s not important to the topic but I’m curious about this since, in instruction selection at least, they both set SelectionDAGISel::OptLevel to CodeGenOpt::None and MCCodeGenInfo::OptLevel by one means or another. Could you point me at a difference between the two? I assume it’s outside of instruction selection.

Without actually doing the research… There are interactions between DAG construction, DAG Combine in particular, possibly CodeGenPrepare, and ISel, which I don’t really understand. We tried to turn off all of DAG Combine, IIRC, but that totally didn’t work, so instead we turn off individual bits and pieces.

We made a distinct effort to find all the IR and MachineFunction passes that don’t run at O0 and turn them off individually (Chandler was particularly insistent that the pass manager should not be aware of ‘optnone’) and it’s always possible that we missed some; those cases should be properly considered bugs. And of course new passes get added from time to time, and typically people don’t think about ‘optnone’ when they’re doing that… again those are properly speaking bugs.

Regarding the “circuit-breaker” analogy, ‘optnone’ itself is a kind of user-visible circuit breaker, and making it not break the entire circuit could be considered a little strange. But as I said, it does seem like a useful debugging tactic for ‘optnone’ itself.

–paulr