Page MenuHomePhabricator

[InferAddrSpace] Teach to handle assumed address space.
ClosedPublic

Authored by hliao on Mon, Nov 9, 9:30 PM.

Details

Summary
  • In certain cases, a generic pointer could be assumed as a pointer to the global memory space or other spaces. With a dedicated target hook to query that address space from a given value, infer-address-space pass could infer and propagate that to all its users.

Diff Detail

Event Timeline

hliao created this revision.Mon, Nov 9, 9:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 9, 9:30 PM
hliao requested review of this revision.Mon, Nov 9, 9:30 PM
hliao updated this revision to Diff 304222.Tue, Nov 10, 8:58 AM

Revise the fix.

hliao updated this revision to Diff 304303.Tue, Nov 10, 12:25 PM

Fix clang-tidy warnings.

hliao added a comment.Wed, Nov 11, 1:53 PM

PING for review

arsenm added inline comments.Wed, Nov 11, 2:52 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
227

We already have a -1 as an invalid addrspace, so optional isn't necessary

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

I would assume !isPointerTy would be unreachable

540

I think this may be too aggressive and should check if it's specifically a kernel argument load. Contant address space should also not be necessary

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
562–570

Won't this cast be naturally inserted with the inferred addres space?

Should also make a note about the restrictions in AMDGPUUsage

hliao added inline comments.Thu, Nov 12, 12:15 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
227

OK. Do we need to document that in the IR langref? That invalid address space ID is not documented anywhere.

hliao added inline comments.Thu, Nov 12, 12:18 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
540

If the variable locates in __constant__ memory, it could only be modified on the host side. It's safe to assume any generic pointer loaded from __constant__ memory is a global one.

hliao added inline comments.Thu, Nov 12, 12:20 PM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
562–570

We need to insert it explicitly as it's previously used as a generic pointer. Without this, we cannot change users of that generic pointer to a global one.

hliao updated this revision to Diff 305154.Fri, Nov 13, 8:09 AM
  • Add a note in the AMDGPU usage document on the assumption made here.
  • Revise the test in clang.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 13, 8:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hliao added inline comments.Fri, Nov 13, 8:11 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
227

Shall we assume this before that reserved invalid address space ID is well-received and documented in LLVM IR ref? I will make either change in this change accordingly.

hliao updated this revision to Diff 305203.Fri, Nov 13, 9:57 AM

Revise the condition check.

hliao marked an inline comment as done.Fri, Nov 13, 9:57 AM
hliao added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
533

With a condition evaluation order revised, we could assert that now.

arsenm added inline comments.Fri, Nov 13, 12:45 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
227

It's not really part of the IR. The address space is a 24-bit unsigned limit and this is already the pass convention

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

I thought we discussed the one possible case where this wasn't necessarily true

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
289–291

This can be return TII->...

llvm/test/Transforms/InferAddressSpaces/AMDGPU/assumed-addrspace.ll
13

Should have test with load from kernel argument

hliao updated this revision to Diff 305250.Fri, Nov 13, 1:31 PM
hliao marked an inline comment as done.

Revise the interface of that target hook.
Add a dedicated test case for value reading from parameter even though most cases are already covered in the clang test.

hliao marked 3 inline comments as done.Fri, Nov 13, 1:38 PM
hliao added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
227

You are right. Forget that totally.

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

For __constant__ memory, the language itself guarantees it won't be changed on the device side. It's very safe to assume that. The frontend should issue an error on any attempt to write it. Also, the runtime and HW is supposed to provide the facility to protect any write into that space from the device side.

I thought the issue we may have the issue is the case for const variable in the global memory space and a const global pointer in the kernel argument. We need to investigate them further to identify potential risks.

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
289–291

good catch. The's the result of the previous Optional<unsigned>. Now, it's revised.

llvm/test/Transforms/InferAddressSpaces/AMDGPU/assumed-addrspace.ll
13

most cases are already captured in the clang test. A new function is added.

hliao marked 3 inline comments as done.Mon, Nov 16, 8:29 AM

Kindly ping for review.

tra added a subscriber: tra.Mon, Nov 16, 9:28 AM
tra added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
533

Is there a suitable constant for don't know result?

hliao added inline comments.Mon, Nov 16, 10:15 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
533

I would say all generic pointers loaded from the constant memory are safe to be assumed as global ones (for AMDGPU). I explained the reason in the usage document. The constant memory could be modified by the host, where only global memory objects are visible.

arsenm accepted this revision.Mon, Nov 16, 1:35 PM

LGTM with nits

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

AMDGPUAS::UNKNOWN_ADDRESS_SPACE

540

This could be a bit broader but is fine for now

541

Ditto

This revision is now accepted and ready to land.Mon, Nov 16, 1:35 PM
arsenm added inline comments.Mon, Nov 16, 1:43 PM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
483

Checking this here looks strange, since it is also catching values that weren't processed as assumed address space

This revision was landed with ongoing or failed builds.Mon, Nov 16, 2:06 PM
This revision was automatically updated to reflect the committed changes.
hliao added inline comments.Mon, Nov 16, 2:24 PM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
483

It stops tracking through instructions generating pointers with assumed AS. It's not necessary to check their operands as the result is assumed. For instructions without assumed AS results, we need to track them through their operands.