[PATCH] D138711: AMDGPU: Remove BufferPseudoSourceValue

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

In D138711#3954893 <https://reviews.llvm.org/D138711#3954893>, @arsenm wrote:

> I was planning on tracking an underlying IR reference wrapped in these. That would require moving PSVs into some kind of dynamically allocatable set owned by the MachineFunction (which I have most of a patch to do somewhere)

Okay. Maybe we can do that once that work actually gets done? As-is, it's just confusing. And at least for some use cases of these we should just use pointers in IR (like the "`addrspace(7)` for buffers" discussion).

> Whatever's done here also applies to ImagePSV

Yeah, I'll happily follow up with a patch that removes ImagePSV as well.



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1052-1054
+    // Fallback address space for use if ptrVal is nullptr. None means unknown
+    // address space.
+    Optional<unsigned> fallbackAddressSpace;
----------------
foad wrote:
> "None means unknown address space" is a noble idea but I don't think it's providing any benefit in practice, since you only use this to initialize MachinePointerInfo, which has no concept of "unknown address space" - it just defaults to address space 0.
How strongly do you feel about that? :)


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2485
+    if (Info.ptrVal)
+      MPI = MachinePointerInfo(Info.ptrVal, Info.offset);
+    else if (Info.fallbackAddressSpace)
----------------
jsilvanus wrote:
> Should we also have passed `Info.offset` before this change? Is the offset always 0?
Yes, I think we should have, though I didn't notice any significant change because of it.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:866
   case PseudoSourceValue::ExternalSymbolCallEntry:
-  case PseudoSourceValue::TargetCustom:
     return AMDGPUAS::CONSTANT_ADDRESS;
----------------
foad wrote:
> Yes this was horrible and misleading. Maybe split the removal of this line (and associated test churn) into a separate patch?
I didn't do that because I don't know if temporarily moving BufferPSV to addrspace(0) would have a negative effect somewhere. Yes, it could be explicitly moved to addrspace(7) here, but... that's sort of what this change does, just in a different way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138711