[PATCH] D83100: [PPC][GlobalISel] Add initial GlobalIsel infrastructure

Mon Jul 6 13:04:32 PDT 2020

kbarton added a comment.

In D83100#2130726 <https://reviews.llvm.org/D83100#2130726>, @tschuett wrote:

> AArch64 has a sub-directory for GlobalISel related things:
>  https://reviews.llvm.org/D81116

I like this idea. I will do the refactoring in a subsequent commit and update the patch, so if others do not like it it's easy to undo.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp:39
+  bool select(MachineInstr &I) override;
+  static const char *getName() { return DEBUG_TYPE; }
+
----------------
arsenm wrote:
> kbarton wrote:
> > arsenm wrote:
> > > I'm pretty sure you don't need these and all the other places that override this are dead code
> > I don't follow this. 
> > Both select and getName seem to be required - getName is needed by the base InstructionSelector implementation in GlobalISel; select is needed by the PPCGenGlobalISel.inc file generated below.
> > 
> > It is entirely possible I'm doing something incorrect though. Could you explain some more?
> Oh right, this isn't the direct pass. I think manual getName overrides are dead code on passes and set by the INITIALIZE_PASS* macros
Yes, that could be. I haven't looked at the implementation for how it works on passes.

Are you OK marking this as done?

================
Comment at: llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp:23
+
+#define DEBUG_TYPE "ppc-isel"
+
----------------
madhur13490 wrote:
> May be ppc-gisel better?
Yes, this is a better idea. Will change it. 

================
Comment at: llvm/lib/Target/PowerPC/PPCLegalizerInfo.h:21
+
+/// This class provides the information for the target register banks.
+class PPCLegalizerInfo : public LegalizerInfo {
----------------
tschuett wrote:
> Legalizer or register banks?
Good catch. Updated to legalizer. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83100