This is an archive of the discontinued LLVM Phabricator instance.

[MIPS GlobalISel] Select sub
ClosedPublic

Authored by Petar.Avramovic on Oct 18 2018, 10:43 PM.

Details

Summary

Lower G_USUBO and G_USUBE. Add narrowScalar for G_SUB.
Legalize and select G_SUB for MIPS 32.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders added inline comments.Oct 19 2018, 8:53 AM
lib/Target/Mips/MipsLegalizerInfo.cpp
29–30 ↗(On Diff #170144)

This shouldn't block this patch, but it would probably be better to implement the G_[US]ADD[OE] instead of doing the equivalent of it in a custom legalization. That would allow you to use .clampScalar(0, s32, s32) here (which would use the default widenScalar and narrowScalar expansions) and the end result is that you'd have support for any length scalar produced by the frontends or optimizers.

lib/Target/Mips/MipsLegalizerInfo.cpp
29–30 ↗(On Diff #170144)

Since there are only a few opcodes supported in default narrowScalar, I used customLegalization.
Also in order to handle both big and little endian, G_MERGE_VALUES and G_UNMERGE_VALUES hold scalars the same way regardelss of endianess, so that customLegalizer knows where scalars with high and low bits are.
My choice was:
%Big = G_MERGE_VALUES %High, %Low
%High, %Low = G_UNMERGE_VALUES %Big
as this option provides most human reader friendly mir output in my opinion (as these are integers and least significant bit is at right hand side when integer is written on screen). Default narrowScalar considers that scalars are held other way around.

Do you think that I should switch to that way, and will all opcodes be supported in narrowScalar?

dsanders added inline comments.Oct 31 2018, 9:22 AM
lib/Target/Mips/MipsLegalizerInfo.cpp
29–30 ↗(On Diff #170144)

Since there are only a few opcodes supported in default narrowScalar, I used customLegalization.

The right thing to do here is to expand on narrowScalar so that narrowScalar supports G_SUB. All targets are going to want the same behaviour.

Also in order to handle both big and little endian, G_MERGE_VALUES and G_UNMERGE_VALUES hold scalars the same way regardelss of endianess, > so that customLegalizer knows where scalars with high and low bits are.
My choice was:
%Big = G_MERGE_VALUES %High, %Low
%High, %Low = G_UNMERGE_VALUES %Big
as this option provides most human reader friendly mir output in my opinion (as these are integers and least significant bit is at right hand side when
integer is written on screen). Default narrowScalar considers that scalars are held other way around.

I agree that going from high-bits to low-bits is a more natural order but at this point the definition is hard to change even if we want to. I don't know the reason behind the choice that was made but the current definition has the nice property that the bit slices are consistent no matter how many operands are present.

gMIR is a target independent representation. The G_* operations do the same thing for all targets and gMIR's vregs have the same layout for all targets. Targets aren't supposed to redefine the semantics G_* opcodes.

Do you think that I should switch to that way?

Yes, you'll be fighting against the common implementation everywhere if you try to redefine G_MERGE_VALUES/G_UNMERGE_VALUES. You'll end up re-implementing the majority of LegalizerHelper and probably quite a bit of CombinerHelper and I would expect future optimization passes to be broken on a target that redefines any of the G_* opcodes.

and will all opcodes be supported in narrowScalar?

All the ones where narrowScalar makes sense. G_INTRINSIC will probably only be partially supported because it can't really support target-specific intrinsics, and narrowScalar is not generally possible for floating point operations without careful analysis w.r.t range and precision.

lib/Target/Mips/MipsLegalizerInfo.cpp
29–30 ↗(On Diff #170144)

The right thing to do here is to expand on narrowScalar so that narrowScalar supports G_SUB. All targets are going to want the same behaviour.

Are other instructions going to be implemented in narrowScalar or I am supposed to add something when I need it?

I agree that going from high-bits to low-bits is a more natural order but at this point the definition is hard to change even if we want to. I don't know the reason behind the choice that was made but the current definition has the nice property that the bit slices are consistent no matter how many operands are present.

Perhaps this should be explicitly stated in GenericOpcodes.td for G_MERGE_VALUES and G_UNMERGE_VALUES.

gMIR is a target independent representation. The G_* operations do the same thing for all targets and gMIR's vregs have the same layout for all targets. Targets aren't supposed to redefine the semantics G_* opcodes.

However, note that G_LOAD and G_STORE in narrowScalar for big-endian don't follow this convention that low-bits go first in G_MERGE/G_UNMERGE.

Yes, you'll be fighting against the common implementation everywhere if you try to redefine G_MERGE_VALUES/G_UNMERGE_VALUES. You'll end up re-implementing the majority of LegalizerHelper and probably quite a bit of CombinerHelper and I would expect future optimization passes to be broken on a target that redefines any of the G_* opcodes.

Thank you for your explanation, I will do that.

dsanders added inline comments.Oct 31 2018, 1:27 PM
lib/Target/Mips/MipsLegalizerInfo.cpp
29–30 ↗(On Diff #170144)

The right thing to do here is to expand on narrowScalar so that narrowScalar supports G_SUB. All targets are going to want the same behaviour.

Are other instructions going to be implemented in narrowScalar or I am supposed to add something when I need it?

Yes, if you need a legalization that's likely to be common to other targets feel then free to create a patch for the LegalizerHelper.

I agree that going from high-bits to low-bits is a more natural order but at this point the definition is hard to change even if we want to. I don't know the reason behind the choice that was made but the current definition has the nice property that the bit slices are consistent no matter how many operands are present.

Perhaps this should be explicitly stated in GenericOpcodes.td for G_MERGE_VALUES and G_UNMERGE_VALUES.

I've added a comment on this

gMIR is a target independent representation. The G_* operations do the same thing for all targets and gMIR's vregs have the same layout for all targets. Targets aren't supposed to redefine the semantics G_* opcodes.

However, note that G_LOAD and G_STORE in narrowScalar for big-endian don't follow this convention that low-bits go first in G_MERGE/G_UNMERGE.

That's because the implementation only accounts for little endian at the moment. That will need to be fixed for a big endian target.

Petar.Avramovic planned changes to this revision.Nov 5 2018, 4:02 AM
Petar.Avramovic edited the summary of this revision. (Show Details)

I'm sorry for the delay. I'll review the patch soon.

This revision is now accepted and ready to land.Jan 26 2019, 10:20 PM
This revision was automatically updated to reflect the committed changes.