Page MenuHomePhabricator

AMDGPU: Remove BufferPseudoSourceValue
ClosedPublic

Authored by nhaehnle on Nov 25 2022, 6:31 AM.

Details

Summary

The use of a PSV for buffer intrinsics is misleading because it may be
misinterpreted as all buffer intrinsics accessing the same address in
memory, which is clearly not true.

Instead, build MachineMemOperands without a pointer value but with an
address space, so that address space-based alias analysis can still
work.

There is a lot of test churn because previously address space 4
(constant address space) was used as an address space for buffer
intrinsics. This doesn't make much sense and seems to have been an
accident -- see the change in
AMDGPUTargetMachine::getAddressSpaceForPseudoSourceKind.

Diff Detail

Event Timeline

nhaehnle created this revision.Nov 25 2022, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 6:31 AM
nhaehnle requested review of this revision.Nov 25 2022, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 6:31 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad accepted this revision.Nov 25 2022, 7:02 AM

A big +1 for the general idea.

llvm/include/llvm/CodeGen/TargetLowering.h
1055–1057

"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.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
791

Yes this was horrible and misleading. Maybe split the removal of this line (and associated test churn) into a separate patch?

This revision is now accepted and ready to land.Nov 25 2022, 7:02 AM

I couldn't spot any nontrivial difference in generated code -- so the new namespace-based info is equally useful (or useless?) for AA?

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2495

Should we also have passed Info.offset before this change? Is the offset always 0?

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)

Whatever's done here also applies to ImagePSV

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.

llvm/include/llvm/CodeGen/TargetLowering.h
1055–1057

How strongly do you feel about that? :)

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2495

Yes, I think we should have, though I didn't notice any significant change because of it.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
791

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.

foad added inline comments.Nov 29 2022, 3:01 AM
llvm/include/llvm/CodeGen/TargetLowering.h
1055–1057

Not very, but if you want to keep it then I think we need a TODO comment where you map "unknown address space" (in IntrinsicInfo) to "address space 0" (in MachinePointerInfo).

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).

I think the hard part I got stuck on was doing anything meaningful in AA without actual IR pointer values

llvm/include/llvm/CodeGen/TargetLowering.h
1055–1057

Might as well make this a plain unsigned address space. If you really want something that breaks hard, ~0u is always invalid

This revision was landed with ongoing or failed builds.Nov 29 2022, 1:15 PM
This revision was automatically updated to reflect the committed changes.