Page MenuHomePhabricator

[amdgpu] Add scope metadata support for noalias kernel arguments.
Needs RevisionPublic

Authored by hliao on Jul 9 2021, 12:15 PM.

Details

Reviewers
arsenm
rampitec
Summary
  • Basically, following the support in function inline, add the scoped alias metadata support in the kernel argument lowering pass.

Diff Detail

Unit TestsFailed

TimeTest
490 msx64 debian > LLVM.CodeGen/AMDGPU::llvm.memcpy.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -march=amdgcn -mcpu=tahiti -verify-machineinstrs < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AMDGPU/llvm.memcpy.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefix=SI -check-prefix=FUNC /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AMDGPU/llvm.memcpy.ll
630 msx64 windows > LLVM.CodeGen/AMDGPU::llvm.memcpy.ll
Script: -- : 'RUN: at line 1'; c:\ws\w7\llvm-project\premerge-checks\build\bin\llc.exe -march=amdgcn -mcpu=tahiti -verify-machineinstrs < C:\ws\w7\llvm-project\premerge-checks\llvm\test\CodeGen\AMDGPU\llvm.memcpy.ll | c:\ws\w7\llvm-project\premerge-checks\build\bin\filecheck.exe -check-prefix=SI -check-prefix=FUNC C:\ws\w7\llvm-project\premerge-checks\llvm\test\CodeGen\AMDGPU\llvm.memcpy.ll

Event Timeline

hliao created this revision.Jul 9 2021, 12:15 PM
hliao requested review of this revision.Jul 9 2021, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 12:15 PM
hliao added a comment.Jul 9 2021, 12:17 PM

So far, this change still depends on two pending changes (D102255 and D102821) to pass all regression tests.

arsenm added a comment.Jul 9 2021, 2:19 PM

Needs some IR tests showing the metadata.

Is this the state of the art for scoped AA? I thought scoped restrict was going to involve new intrinsics (which we could also insert up front if we were consistently using byref to begin with)

llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp
148

Do you really need to go through all the instruction types? I think there's a utility function for this somewhere

159

I think this can be a bit smarter with intrinsics (ideally the helper would handle this too) . We already have to report which of the arguments are pointers

162

Should check the callsite in case it's marked readnone, not the call itself

arsenm requested changes to this revision.Aug 6 2021, 8:34 AM
This revision now requires changes to proceed.Aug 6 2021, 8:34 AM

Do not duplicate the logic from the inliner here, but refactor and reuse the AddNoAliasScopeMetadata from InlineFunction.cpp. That must allow the functionality to stay in sync with each other. It should also make it easier in preparation for the full restrict changes.

llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
1262–1263

I am also wondering why those two stores are referring to the same noalias set ? In the current noalias scope handling, I would expect something like:

; HSA-NEXT:    store volatile i8 addrspace(1)* [[PTR0_LOAD]], i8 addrspace(1)* addrspace(1)* undef, align 8, !alias.scope !42, !noalias !43
; HSA-NEXT:    store volatile i8 addrspace(1)* [[PTR1_LOAD]], i8 addrspace(1)* addrspace(1)* undef, align 8, !alias.scope !43, !noalias !42

indicating that the two stores are exclusive ?

1937

Also add the HSA/MESA checks for the noalias-scope related metadata.

I am also wondering if things would become easier if the pass was restructured to create a wrapper function that contains the replacement of the arguments, and then depending on llvm::InlineFunction (the call, not the inliner pass) for the propagation of those values to the actual kernel ? That should handle the noalias attribute, as well maybe as other attributes in a clean way ?