This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Rename amdgcn_wwm to amdgcn_strict_wwm
ClosedPublic

Authored by piotr on Feb 8 2021, 4:25 AM.

Details

Summary
  • Introduce the new intrinsic amdgcn_strict_wwm
  • Deprecate the old intrinsic amdgcn_wwm

The change is done for consistency as the "strict"
prefix will become an important, distinguishing factor
between amdgcn_wqm and amdgcn_strictwqm in the future.

The "strict" prefix indicates that inactive lanes do not
take part in control flow, specifically an inactive lane
enabled by a strict mode will always be enabled irrespective
of control flow decisions.

The amdgcn_wwm will be removed, but doing so in two steps
gives users time to switch to the new name at their own pace.

Diff Detail

Event Timeline

piotr created this revision.Feb 8 2021, 4:25 AM
piotr requested review of this revision.Feb 8 2021, 4:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 4:25 AM
critson accepted this revision.Feb 8 2021, 7:45 PM

LGTM, seems to be straightforward refactor.
Is it worth adding a test to ensure old intrinsic still works?

I do start to wonder if it is should be "strictwwm" or "strict.wwm".
However, we do have "softwqm" already, and hardware convention seems to be generally to glue words together, e.g. "readlane", "readfirstlane", etc -- so "strictwwm" is probably right.
SOFT_WQM pseudo should probably become SOFTWQM for consistency at some point.

This revision is now accepted and ready to land.Feb 8 2021, 7:45 PM
piotr added a comment.Feb 9 2021, 5:28 AM

Is it worth adding a test to ensure old intrinsic still works?

The old intrinsic is still supported, so I agree we should keep the tests. I will add them back.

I do start to wonder if it is should be "strictwwm" or "strict.wwm".
However, we do have "softwqm" already, and hardware convention seems to be generally to glue words together, e.g. "readlane", "readfirstlane", etc -- so "strictwwm" is probably right.
SOFT_WQM pseudo should probably become SOFTWQM for consistency at some point.

Yeah, I noticed that too. I agree SOFT_WQM is the odd one out. I will make it consistent in a separate change.

arsenm added inline comments.Feb 9 2021, 8:52 AM
llvm/include/llvm/IR/IntrinsicsAMDGPU.td
1615

Would somewhat prefer a _/. between strict and wwm

piotr added inline comments.Feb 10 2021, 12:56 AM
llvm/include/llvm/IR/IntrinsicsAMDGPU.td
1615

I don't have a strong opinion myself. I can add "_" if that is easier to parse (and leave soft_wqm as it is).

piotr updated this revision to Diff 323763.Feb 15 2021, 8:09 AM

Add the underscore between strict and wwm. Restored the old tests for amdgcn_wwm.

piotr retitled this revision from [AMDGPU] Rename amdgcn_wwm to amdgcn_strictwwm to [AMDGPU] Rename amdgcn_wwm to amdgcn_strict_wwm.Feb 15 2021, 8:13 AM
piotr edited the summary of this revision. (Show Details)
piotr updated this revision to Diff 325429.Feb 22 2021, 6:09 AM

Rebased and updated tests.

critson added inline comments.Feb 23 2021, 1:15 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
122

llvm.amdgcn.strict.wwm

piotr updated this revision to Diff 325711.Feb 23 2021, 1:37 AM

Added missing dot in the intrinsic name.

This revision was landed with ongoing or failed builds.Mar 3 2021, 1:16 AM
This revision was automatically updated to reflect the committed changes.