[PATCH] D131296: [PowerPC] Add support for extending and truncating values

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:107
+  APInt ConstValue = I.getOperand(1).getCImm()->getValue();
+  if (ConstValue.isIntN(16)) {
+    bool NeedMask = !ConstValue.isIntN(15);
----------------
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


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:213
+    return selectConst(I, MBB, MRI);
+  case TargetOpcode::G_SEXT_INREG:
+    return selectSExt(I, MBB, MRI);
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:214
+  case TargetOpcode::G_SEXT_INREG:
+    return selectSExt(I, MBB, MRI);
+  default:
----------------
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


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:38
   case PPC::G8RC_and_G8RC_NOX0RegClassID:
+  case PPC::G8pRCRegClassID:
+  case PPC::G8pRC_with_sub_32_in_GPRC_NOR0RegClassID:
----------------
This is for i128? Seems no related handling for i128 in this patch?


================
Comment at: llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-constant.ll:1
+; RUN: llc -mtriple ppc64le-linux -global-isel -o - < %s | FileCheck %s -check-prefixes=CHECK,LINUX
+
----------------
amyk wrote:
> nit: Might be good to autogenerate these LIT tests with `-ppc-asm-full-reg-names`.
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?


================
Comment at: llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-logical.ll:2
 ; RUN: llc -mtriple ppc64le-linux -global-isel -o - < %s | FileCheck %s -check-prefixes=CHECK,LINUX
 
 ; CHECK-LABEL: test_andi64:
----------------
This test file seems not belong to this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131296/new/

https://reviews.llvm.org/D131296