This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Emit metadata for the hidden_multigrid_sync_arg conditionally
ClosedPublic

Authored by cfang on Apr 11 2022, 3:02 PM.

Details

Summary

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.

Diff Detail

Event Timeline

cfang created this revision.Apr 11 2022, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 3:02 PM
cfang requested review of this revision.Apr 11 2022, 3:02 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
cfang added a reviewer: bcahoon. cfang removed 1 blocking reviewer(s): sameerds.Apr 11 2022, 3:03 PM
cfang added a reviewer: Restricted Project.
b-sumner added inline comments.Apr 11 2022, 4:47 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
545

I expected to see getMultigridSyncArgImplicitArgPosition used here.

cfang marked an inline comment as done.Apr 11 2022, 5:19 PM
cfang added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
545

Thanks. I was thinking whether we should only limit the change to v5
and not sure how did I end up with the garbage code. Will correct it.

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

cfang updated this revision to Diff 422083.Apr 11 2022, 5:53 PM
cfang marked an inline comment as done.
  1. Correct the function funcRetrievesMultigridSyncArg;
  2. update after rebase.
sameerds accepted this revision.Apr 11 2022, 11:43 PM

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.

This revision is now accepted and ready to land.Apr 11 2022, 11:43 PM
foad added a comment.Apr 12 2022, 2:39 AM

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

859

I think llvm style is to put braces around the "else" part if there are braces around the "if" part.

arsenm added inline comments.Apr 12 2022, 6:13 AM
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?

cfang added inline comments.Apr 12 2022, 11:24 AM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
439

This is a good suggestion. But I would rather postpone it to another patch.
I need time to investigate all the possible complexities we might encounter.
For example, two arguments shares the same offset in the same code object version, one argument has different offsets for different code object versions.
I also need to understand how to do all checks in one call.

cfang added inline comments.Apr 12 2022, 11:41 AM
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.

859

Really? I have to check llvm style ( I was thinking clang-format will complain if something is wrong).

cfang marked an inline comment as done.Apr 12 2022, 12:38 PM

Typos in description: "and also and also", "WE".

Thanks. Updated in the commit message.

This revision was landed with ongoing or failed builds.Apr 13 2022, 2:31 PM
This revision was automatically updated to reflect the committed changes.