[PATCH] D57399: AMDGPU/GlobalISel: Add support for wide loads >= 256-bits

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def:185-188
+  /* FIXME: The generic register bank select does not support complex
+   * break downs where the number of vector elements does not equal the
+   * number of breakdowns.
+   */
----------------
I have most of a patch for this


================
Comment at: lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def:209
+
+  assert ((Size == 256 || Size == 512) && BankID == AMDGPU::VGPRRegBankID);
+
----------------
Extra space before (


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:141
+}
+
+
----------------
extra newline


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:198-199
+    LLT LoadTy = MRI.getType(MI.getOperand(0).getReg());
+    // FIXME: Is it possible to physreg outputs at this point?  If not we don't
+    // need to check LoadTy.isValid().
+    if (!LoadTy.isValid())
----------------
No, the verifier rejects physical registers in G_ instructions


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:407
+  case AMDGPU::G_LOAD: {
+    unsigned DstReg = MI.getOperand(0).getReg();
+    unsigned LoadSize = MRI.getType(DstReg).getSizeInBits();
----------------
This should be split to a new function


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:451-454
+      MachineMemOperand *SplitMMO = B.getMF().getMachineMemOperand(
+          MMO.getPointerInfo().getWithOffset(OffsetBytes), MMO.getFlags(),
+          MaxNonSmrdLoadSize / 8, Alignment, MMO.getAAInfo(), MMO.getRanges(),
+          MMO.getSyncScopeID(), MMO.getOrdering(), MMO.getFailureOrdering());
----------------
You can do just MF.getMachineMemOperand(MMO, Offset, Size). I hit a bug in it though for D57122


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:569-570
+  LLT LoadTy = MRI.getType(MI.getOperand(0).getReg());
+  // FIXME: Is it possible to physreg outputs at this point?  If not we don't
+  // need to check LoadTy.isValid().
+  if (!LoadTy.isValid())
----------------
No, the verifier rejects physical registers in G_ instructions


================
Comment at: test/CodeGen/AMDGPU/GlobalISel/regbankselect-load.mir:4
+
+# REQUIRES: global-isel
+
----------------
This isn't needed anymore


================
Comment at: test/CodeGen/AMDGPU/GlobalISel/regbankselect-load.mir:7
+--- |
+  define amdgpu_kernel void @load_global_v8i32_non_uniform(<8 x i32> addrspace(1)* %in) {
+    %tmp0 = call i32 @llvm.amdgcn.workitem.id.x() #0
----------------
I want to split the memory tests per address space, so split this into regbankselect-load-global, regbankselect-load-constant


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57399