[PATCH] D26730: AMDGPU/GlobalISel: Add support for simple shaders

[Matt Arsenault via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUCallLowering.cpp:49
+  const SIRegisterInfo *TRI =
+      static_cast<const SIRegisterInfo*>(MF.getSubtarget().getRegisterInfo());
+  MachineRegisterInfo &MRI = MF.getRegInfo();
----------------
getSubtarget<SISubtarget>


================
Comment at: lib/Target/AMDGPU/AMDGPUCallLowering.cpp:96
+  MachineFunction &MF = MIRBuilder.getMF();
+  const SISubtarget *Subtarget = static_cast<const SISubtarget *>(&MF.getSubtarget());
+  MachineRegisterInfo &MRI = MF.getRegInfo();
----------------
getSubtarget<SISubtarget>


================
Comment at: lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def:34
+
+RegisterBankInfo::PartialMapping PartMappings[] {
+  // StartIdx, Legnth, RegBank
----------------
const


================
Comment at: lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def:35
+RegisterBankInfo::PartialMapping PartMappings[] {
+  // StartIdx, Legnth, RegBank
+  {0, 32, SGPRRegBank},
----------------
Typo Length


================
Comment at: lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def:42
+
+RegisterBankInfo::ValueMapping ValMappings[] {
+  // SGPR 32-bit
----------------
const


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:36
+    : InstructionSelector(), TII(*STI.getInstrInfo()),
+      TRI(*static_cast<const SIRegisterInfo*>(STI.getRegisterInfo())),
+      RBI(RBI) {}
----------------
This should probably be the type of the subtarget to begin with


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:87-93
+  BuildMI(*BB, &I, DL, TII.get(AMDGPU::S_ADD_U32), DstLo)
+          .addOperand(getSubOperand64(I.getOperand(1), AMDGPU::sub0))
+          .addOperand(getSubOperand64(I.getOperand(2), AMDGPU::sub0));
+
+  BuildMI(*BB, &I, DL, TII.get(AMDGPU::S_ADDC_U32), DstHi)
+          .addOperand(getSubOperand64(I.getOperand(1), AMDGPU::sub1))
+          .addOperand(getSubOperand64(I.getOperand(2), AMDGPU::sub1));
----------------
Can't the register bank be checked here directly to switch to v_add?


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:222
+  // PseudoSourceValue like GOT.
+  if (!Ptr || isa<UndefValue>(Ptr) || isa<Argument>(Ptr) ||
+      isa<Constant>(Ptr) || isa<GlobalValue>(Ptr))
----------------
I'm not sure if Ptr == null is valid


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:293
+  unsigned DstReg = I.getOperand(0).getReg();
+  DebugLoc DL = I.getDebugLoc();
+  unsigned Opcode;
----------------
const &


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:354-355
+  unsigned DstReg, PtrReg;
+  DstReg = I.getOperand(0).getReg();
+  PtrReg = I.getOperand(1).getReg();
+  unsigned LoadSize = RBI.getSizeInBits(DstReg, MRI, TRI);
----------------
Define and declare at same type


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:391
+bool AMDGPUInstructionSelector::select(MachineInstr &I) const {
+  DEBUG(dbgs() << "AMDGPU: select\n");
+  DebugLoc DL = I.getDebugLoc();
----------------
This is probably noisy


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:392
+  DEBUG(dbgs() << "AMDGPU: select\n");
+  DebugLoc DL = I.getDebugLoc();
+
----------------
Unused variable 


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:28
+class AMDGPUInstructionSelector : public InstructionSelector
+   {
+public:
----------------
extra line


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:33
+
+  virtual bool select(MachineInstr &I) const override;
+
----------------
-virtual


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:48
+  bool selectG_GEP(MachineInstr &I) const;
+  bool hasVgprParts(const SmallVectorImpl<GEPInfo> &AddrInfo) const;
+  void getAddrModeInfo(const MachineInstr &Load, const MachineRegisterInfo &MRI,
----------------
ArrayRef?


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:51
+                       SmallVectorImpl<GEPInfo> &AddrInfo) const;
+  bool selectSMRD(MachineInstr &I, const SmallVectorImpl<GEPInfo> &AddrInfo) const;
+  bool selectG_LOAD(MachineInstr &I) const;
----------------
ArrayRef?


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.h:26-28
+  SGPRRegBankID = 0, /// General Purpose Registers: W, X.
+  VGPRRegBankID = 1, /// Floating Point/Vector Registers: B, H, S, D, Q.
+  CCRRegBankID = 2, /// Conditional register: NZCV.
----------------
Comments are leftover copy paste


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:279
+    GISel->Legalizer.reset(new AMDGPULegalizerInfo());
+    assert(GISel->Legalizer);
+
----------------
There's no reason to check this


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:281-283
+    auto *RBI = new AMDGPURegisterBankInfo(*I->getRegisterInfo());
+    GISel->InstSelector.reset(new AMDGPUInstructionSelector(*I, *RBI));
+    GISel->RegBankInfo.reset(RBI);
----------------
RBI should be created first direct to the reset, then you can avoid the temporary naked pointer


https://reviews.llvm.org/D26730