Page MenuHomePhabricator

GlobalISel: Allow shift amount to be a different type
ClosedPublic

Authored by arsenm on Dec 12 2018, 7:15 PM.

Details

Summary

For AMDGPU the shift amount is never 64-bit, and
this needs to use a 32-bit shift.

An equivalent of getShiftAmountTy is necessary for the combiner, but
I'm not sure where to put it.

I'm sort of guessing on the x86 changes. Currently
in SelectionDAG the shift amount is i8. The global
isel selector seems to have been using some kind of
hack to get the 8-bit subregister, which breaks with
this change. The copy it was inserting was invalid
because it ended up being a copy from a super-register
to its own subregister.

Diff Detail

Event Timeline

arsenm created this revision.Dec 12 2018, 7:15 PM
volkan added a subscriber: volkan.Dec 19 2018, 10:20 AM

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
113

Use the SrcTy here?

arsenm added a comment.Jan 7 2019, 5:06 PM

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

arsenm marked an inline comment as done.Jan 7 2019, 11:00 PM
arsenm added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
113

It's required to be the same, so I don't see the point? The problematic part is the shift amount src type, since there was no shift to begin with

igorb added a comment.Jan 8 2019, 5:09 AM

The x86 changes looks ok. Few minor comments.
Thanks.

lib/Target/X86/X86InstructionSelector.cpp
1532–1533

please remove CReg , not in use any more.

1545

please add assert, insure Operand(2) size is 8bit

lib/Target/X86/X86RegisterBankInfo.cpp
187

please remove empty line

189

This code is similar to getSameOperandsMapping(..) .
Could you please modify getSameOperandsMapping to handle this case?

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

For AArch64 we want shift immediates to be 64 bit in order to select using the imported patterns. I can see 3 options:
a) we ask the target via a hook what the preferred shift immediate type is
b) we iteratively check from i64->i32(maybe -> i16?) using the isInstUnsupported() call to see which type combination is legal and pick the first
c) we don't worry about the type and get the legalizer to re-legalize it. I don't think however that the legalizer artifact combiner currently tries to re-legalize the combined instructions. @aditya_nandakumar do you have an opinion?

For now, my slight preference would be option b) as its simpler and doesn't try to change the way the legalizer/combiner currently works.

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

For AArch64 we want shift immediates to be 64 bit in order to select using the imported patterns. I can see 3 options:
a) we ask the target via a hook what the preferred shift immediate type is
b) we iteratively check from i64->i32(maybe -> i16?) using the isInstUnsupported() call to see which type combination is legal and pick the first
c) we don't worry about the type and get the legalizer to re-legalize it. I don't think however that the legalizer artifact combiner currently tries to re-legalize the combined instructions. @aditya_nandakumar do you have an opinion?

For now, my slight preference would be option b) as its simpler and doesn't try to change the way the legalizer/combiner currently works.

This does assume a power of 2 legality

arsenm marked 3 inline comments as done.Jan 10 2019, 4:06 AM
arsenm added inline comments.
lib/Target/X86/X86InstructionSelector.cpp
1545

I think this is redundant since the verifier will catch the mismatched sizes with COPY to CL

lib/Target/X86/X86RegisterBankInfo.cpp
189

I wasn't sure what the "Same" was referring to here. Since it specifically asserts for mismatched types, I figured this for some reason cared about operands all with the same size. It probably needs a new name, but I'm not sure what it should be here without knowing more about x86's register banks.

Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?

Do you have an opinion how how to handle selecting the shift amount type in the combiner?

For AArch64 we want shift immediates to be 64 bit in order to select using the imported patterns. I can see 3 options:
a) we ask the target via a hook what the preferred shift immediate type is
b) we iteratively check from i64->i32(maybe -> i16?) using the isInstUnsupported() call to see which type combination is legal and pick the first
c) we don't worry about the type and get the legalizer to re-legalize it. I don't think however that the legalizer artifact combiner currently tries to re-legalize the combined instructions. @aditya_nandakumar do you have an opinion?

For now, my slight preference would be option b) as its simpler and doesn't try to change the way the legalizer/combiner currently works.

This does assume a power of 2 legality

I spoke with Aditya offline and the legalizer combines will actually retry legalization of any instructions generated by the combine. This means we could use option c), and just emit the shift with the default type, and rely on the target's legalization rules to morph it into the best form.

arsenm updated this revision to Diff 182242.Jan 17 2019, 3:25 AM
aemerson accepted this revision.Jan 22 2019, 1:19 PM

LGTM, thanks.

This revision is now accepted and ready to land.Jan 22 2019, 1:19 PM
arsenm closed this revision.Jan 22 2019, 1:42 PM

r351882