Adds/completes support for G_SEXT, G_ZEXT, and G_TRUNC.
Support for G_SEXT and G_ZEXT was already partly implemented.
Details
Diff Detail
Event Timeline
Did I miss the tests for G_ANYEXT and G_TRUNC?
Apparently, the solution are MIR-tests like, e.g.,
https://reviews.llvm.org/D37675
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp | ||
---|---|---|
209 | Do we need to check if these are valid registers first before getting the register? | |
llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-constant.ll | ||
1 ↗ | (On Diff #450411) | nit: Might be good to autogenerate these LIT tests with -ppc-asm-full-reg-names. |
llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-logical.ll | ||
11 ↗ | (On Diff #450411) | nit: Might be good to autogenerate these LIT tests with -ppc-asm-full-reg-names here, as well. |
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp | ||
---|---|---|
144 | For imm32, seems the implementation in PPCFastISel::PPCMaterialize32BitInt() looks simpler to me. int64_t Imm = ConstValue.getZExtValue(); if (isInt<16>(Imm)) { // a single LI/LI8 is enough } else if (Imm & 0xFFFF) { // Handle the low and high 16 bit seperatedly } else { // only handle the high 16 as the low 16 bits are all 0 } And for the 64 bit integers, maybe we can also refer to PPCFastISel::PPCMaterialize64BitInt() if we want to have a implementation for them | |
426 | Seems the logic in selectSExt is not what G_SEXT_INREG represents. Can we set action for G_SEXT_INREG as lower and use target independent handling for G_SEXT_INREG like other targets does? Not sure which test case triggers this generic opcode? | |
427 | G_SEXT seems can be selected automatically in the td files but G_ZEXT can not. So I suppose there should be some codes to select G_ZEXT too? I made a implementation in https://reviews.llvm.org/D135535 | |
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp | ||
38 | This is for i128? Seems no related handling for i128 in this patch? | |
llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-constant.ll | ||
1 ↗ | (On Diff #450411) | And seems the cases are all testing zero/sign extension from i32 -> i64. Should we test other code path as well, like i8/i16 -> i64? And for trunc operation too? |
llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-logical.ll | ||
2 ↗ | (On Diff #450411) | This test file seems not belong to this patch? |
Completely reworked the change.
- Rebased on latest main which has partial implementation of G_ZEXT (Currently, G_ZEXT and G_SEXT are only implemented for extending 32 -> 64 bit.)
- Removed support for G_CONSTANT. I will have a more complete solution ready in a follow up change.
- Added a mir test case to check the generated code
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp | ||
---|---|---|
247 | What will happen if we don't constrain the middle result, like ImpDefReg and result of INSERT_SUBREG? I assume the register class setting in the source code here should match what IMPLICIT_DEF INSERT_SUBREG and the final RLDICL requires? | |
254 | Do we need to handle any arbitrary bit here, like i7/i9? I assume the illegal types should be already handled in legalizer pass? | |
258 | hmm, what if the register class of SrcReg is not g8rc? Directly putting it as operand of PPC::RLDICL seems not right to me. For example if the input is gprc, constraining may change it to g8rc by adding a COPY? but that COPY will not tell how to handle the high 32 bit? | |
284 | nit: sounds like here should be dest operand? | |
292 | We may need to change the tb for the 8 and 16 bits in another patch. For example add patterns for EXTSB8_32_64 and EXTSH8_32_64? | |
336 | Weird that trunc i64 to i32 can not be handled in table gen? def : Pat<(i32 (trunc i64:$in)), (EXTRACT_SUBREG $in, sub_32)>; Seems the patterns in the match table are all about trunc from i64/i32 -> i1. Maybe a closely look is needed to understand why for later improvement. | |
342 | Same as sext, directly putting SrcReg as operand of PPC::RLDICL will cause copy like %1:gprc = COPY %3:g8rc and this COPY can not be expanded in later pseudo expansion. |
For imm32, seems the implementation in PPCFastISel::PPCMaterialize32BitInt() looks simpler to me.
And for the 64 bit integers, maybe we can also refer to PPCFastISel::PPCMaterialize64BitInt() if we want to have a implementation for them