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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUCallLowering.cpp:1
 //===-- llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp - Call lowering ---===//
 //
----------------
Is this line 1-char short?


================
Comment at: lib/Target/AMDGPU/AMDGPUCallLowering.cpp:38
 bool AMDGPUCallLowering::lowerReturn(MachineIRBuilder &MIRBuilder,
                                         const Value *Val, unsigned VReg) const {
+  MIRBuilder.buildInstr(AMDGPU::S_ENDPGM);
----------------
Indentation.


================
Comment at: lib/Target/AMDGPU/AMDGPUCallLowering.cpp:39
                                         const Value *Val, unsigned VReg) const {
+  MIRBuilder.buildInstr(AMDGPU::S_ENDPGM);
   return true;
----------------
Is the returned vreg ignored because all functions are void?


================
Comment at: lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1144-1145
   int64_t ByteOffset = C->getSExtValue();
-  int64_t EncodedOffset = Gen < AMDGPUSubtarget::VOLCANIC_ISLANDS ?
-      ByteOffset >> 2 : ByteOffset;
+  int64_t EncodedOffset =
+      SIInstrInfo::getSMRDEncodedOffset(*Subtarget, ByteOffset);
 
----------------
Unrelated change?


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.h:216
+
+  CCAssignFn *CCAssignFnForCall(CallingConv::ID CC, bool IsVarArg) const;
 };
----------------
FWIW I'd keep this in CallLowering (maybe hardcoded?), as you're not already using CCAssignFn in ISelLowering anyway.


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:36-37
+    : InstructionSelector(), TII(*STI.getInstrInfo()),
+      TRI(*STI.getRegisterInfo()),
+      RBI(RBI) {}
+
----------------
Re-indent?


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:40
+MachineOperand AMDGPUInstructionSelector::getSubOperand64(MachineOperand &MO,
+                                      unsigned SubIdx) const {
+
----------------
Indentation.


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:184
+    return;
+  }
+
----------------
Unnecessary braces.


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:18
+#include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
+#include "llvm/CodeGen/MachineInstr.h"
+
----------------
Forward-decl MachineInstr/MachineOperand too?


================
Comment at: lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:53
+  setAction({G_STORE, S32}, Legal);
+  setAction({G_STORE, 1, S64}, Legal);
+  setAction({G_STORE, 1, P1}, Legal);
----------------
Huh, why are non-pointer types legal?


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.h:43-48
+  /// Get the cost of a copy from \p B to \p A, or put differently,
+  /// get the cost of A = COPY B. Since register banks may cover
+  /// different size, \p Size specifies what will be the size in bits
+  /// that will be copied around.
+  ///
+  /// \note Since this is a copy, both registers have the same size.
----------------
Remove the redundant comments? (I fixed AArch64 in r289844)


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:241
+  }
+  const class LegalizerInfo *getLegalizerInfo() const override {
+    return Legalizer.get();
----------------
'class' isn't necessary anymore.  Also fixed the other targets in r289848 ;)


================
Comment at: lib/Target/AMDGPU/CMakeLists.txt:42
   AMDGPUISelDAGToDAG.cpp
+  AMDGPULegalizerInfo.cpp
   AMDGPUMCInstLower.cpp
----------------
You should only includes these files if LLVM_BUILD_GLOBAL_ISEL=ON.  AArch64/CMakeLists.txt has an example of the cmake goop you'll need.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:615
 
-  const Instruction *I = dyn_cast<Instruction>(Ptr);
-  return I && I->getMetadata("amdgpu.uniform");
+  return AMDGPU::isUniformMMO(MemNode->getMemOperand());
 }
----------------
Maybe commit this and the InstrInfo changes separately?


https://reviews.llvm.org/D26730