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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

I'd recommend splitting out a few tests for the hairier parts of RBS, and you folks know the target specifics, but otherwise, this looks good to me.



================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.h:39
+
+  struct GEPInfo {
+    const MachineInstr &GEP;
----------------
Also private?


================
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);
----------------
tstellarAMD wrote:
> arsenm wrote:
> > ab wrote:
> > > arsenm wrote:
> > > > ab wrote:
> > > > > Huh, why are non-pointer types legal?
> > > > What do you mean?
> > > Why is it legal to have an 's64' pointer (#1) operand to G_STORE/G_LOAD/G_GEP?
> > Oh, I didn't realize integers and pointers were distinct here. 32-bit and 64-bit pointers should both be legal
> RegBankSelect will only create copies with scalar types, so if the pointer operands need to be copied to a new register bank, the new value will have a scalar type and not a pointer type.
Huh, ..that's a bug.  In particular, it means that mapping a vector operation to two parts will lose the vector type, and that's plain incorrect.  We'll have to look into that; expressing the mappings in terms of types instead of size should be sufficient.


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:142
+    // FIXME: Don't hard code pointer size.
+    PtrMapping = AMDGPU::getValueMapping(AMDGPU::SGPRRegBankID, 64);
+  } else {
----------------
You can getSizeInBits() on the type of the pointer operand as well; they always have both addressspace and size.


================
Comment at: lib/Target/AMDGPU/AMDGPURegisterBanks.td:9-11
+//
+//
+//===----------------------------------------------------------------------===//
----------------
Add a comment, or remove the block?


================
Comment at: test/CodeGen/AMDGPU/GlobalISel/inst-select-load-flat.mir:13-15
+registers:
+  - { id: 0, class: vgpr }
+  - { id: 1, class: vgpr }
----------------
This isn't necessary anymore: you can now put the bank/class at the first definition:

    %0:vgpr(p1) = COPY %vgpr0_vgpr1


https://reviews.llvm.org/D26730