[PATCH] D134792: [PowerPC][GISel] support 64 bit load/store

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:141
+    switch (OpSize) {
+    case 32:
+      return isStore ? PPC::STFS : PPC::LFS;
----------------
tschuett wrote:
> Did you say 64bit?
Sure, I can remove this. There is no user for this now.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:264-265
+    return false;
+  case TargetOpcode::G_LOAD:
+  case TargetOpcode::G_STORE: {
+    GLoadStore &LdSt = cast<GLoadStore>(I);
----------------
arsenm wrote:
> What's the problem with importing the tablegen patterns? This looks pretty simple?
PPC does not have a single rule for the simple pattern below in td files, it uses one rule to match all patterns in the PPCTargetLowering.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:153-155
+/// FIXME: this is copied from target AArch64. Needs some code refactor here to
+/// put this function in class RegisterBankInfo.
+static bool isPreISelGenericFloatingPointOpcode(unsigned Opc) {
----------------
arsenm wrote:
> Should put in utils 
I will post another NFC patch to refactor these functions.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:168
+  case TargetOpcode::G_FNEARBYINT:
+  case TargetOpcode::G_FNEG:
+  case TargetOpcode::G_FCOS:
----------------
arsenm wrote:
> I wouldn't really call FNEG or FABS FP opcodes
Sorry, I do not quite understand. Do you mean we should not add `G_FNEG` and `G_FABS` here? But these two opcodes are floating pointer operations, with one floating point input and one floating point output?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134792