This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add support for extending and truncating values
Needs ReviewPublic

Authored by Kai on Aug 5 2022, 2:42 PM.

Details

Summary

Adds/completes support for G_SEXT, G_ZEXT, and G_TRUNC.
Support for G_SEXT and G_ZEXT was already partly implemented.

Diff Detail

Event Timeline

Kai created this revision.Aug 5 2022, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 2:42 PM
Kai requested review of this revision.Aug 5 2022, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 2:42 PM
tschuett added a comment.EditedAug 6 2022, 6:57 AM

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

amyk added a subscriber: amyk.Dec 8 2022, 7:01 AM
amyk added inline comments.
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.

shchenz added inline comments.Dec 12 2022, 3:35 AM
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?

Kai updated this revision to Diff 482956.Dec 14 2022, 12:03 PM
Kai edited the summary of this revision. (Show Details)

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
Kai added a reviewer: shchenz.Dec 14 2022, 6:59 PM
shchenz added inline comments.Dec 21 2022, 7:55 PM
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?
There is pattern:

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.