Introduce a new function attribute, amdgpu-no-multigrid-sync-arg, which is default.
We use implicitarg_ptr + offset to check whether the multigrid synchronization
pointer is used. If yes, we remove this attribute and also and also remove
amdgpu-no-implicitarg-ptr. WE generate metadata for the hidden_multigrid_sync_arg
only when the amdgpu-no-multigrid-sync-arg attribute is removed from the function.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp | ||
---|---|---|
545 | I expected to see getMultigridSyncArgImplicitArgPosition used here. |
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp | ||
---|---|---|
545 | Thanks. I was thinking whether we should only limit the change to v5 The reason I was not sure for pre-v5 is that printf and hostcall hold the same offset, so it is not possible to use implicitarg_ptr + offset to detect. To say that, our hostcall_ptr check logic is broken because even though you are doing printf, you may still end up hostcall metadata emission ( for earlier code object versions). |
This patch clearly follows the same pattern as other attributes. At least I didn't spot anything different. Please give it a day for others to comment, otherwise good to go.
Typos in description: "and also and also", "WE".
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
429 | Personally I would prefer to swap the "if" and "else" parts, since "if (!cond) else ..." reads like a double negative. (Actually a triple negative since it's testing a negative attribute!) | |
856 | I think llvm style is to put braces around the "else" part if there are braces around the "if" part. |
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp | ||
---|---|---|
439 | Since we're going to start adding a lot of these, should we do checks for all the different fields in one forallInterferingAccesses call? |
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp | ||
---|---|---|
439 | This is a good suggestion. But I would rather postpone it to another patch. |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
429 | I agree with you in the general sense. But this case seems a little special. The logic is if (function has multigrid... attribute) emit metadata for multigrid. However, the name of the attribute is amdgpu-no-multigrid-sync-arg, and thus we use a negate to exactly represent the logic. In addition, the "else" part essentially does nothing (or things just like cleaning up, or adjusting offset in v5), exchanging "if" and "else" parts looks a little weird because we are more interested in the "if" part. Since we are doing in this style in this file all the time, I am not going to change in this patch. But this is a good point of suggestion. Thanks. | |
856 | Really? I have to check llvm style ( I was thinking clang-format will complain if something is wrong). |
Since we're going to start adding a lot of these, should we do checks for all the different fields in one forallInterferingAccesses call?