This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX11] Add VOPD VGPR bank access validation
ClosedPublic

Authored by dp on Sep 30 2022, 6:53 AM.

Details

Summary

Define helpers for querying properties of VOPD components and VOPD validation.

The proposed helpers may also be used to simplify VOPD handling in codegen.
To minimize review size codegen changes will be added by a separate patch.

Diff Detail

Event Timeline

dp created this revision.Sep 30 2022, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 6:53 AM
dp requested review of this revision.Sep 30 2022, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 6:53 AM
dp updated this revision to Diff 464555.Oct 2 2022, 8:41 AM

Refactor to remove some hardcoded values.

Joe_Nash accepted this revision.Oct 3 2022, 9:37 AM

Except for a few nits and questions, this patch LGTM. The simplification in AsmParser is quite elegant. @foad please take a look at the interface design in AMDGPUBaseInfo.h

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1667

This is checking reg bank constraints only, so rename to validateVOPDRegBankConstraints. We check constant bus limits elsewhere.

3591

VGPR32 may be an issue for V_DUAL_DOT2ACC_F32_F16 as True16 work progresses, can you make it any VGPR?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
507

When is 0 the expected result? Can you add a comment?

552

Is implicitly converting the return value by constructing good c++ style? I have no idea if it is or not, but it took me a minute to realize it happened.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
544

Name this FIRST_MC_SRC_IDX ?

553

rename to FIRST_PARSED_SRC_IDX

595

This method appears unused, and it is a bit confusingly named, contrasted with getNumOperands

611

Method name is confusing. Maybe it should be isIdxMandatoryLiteralOp

651

It seems like GetRegIdx does not need Component as a parameter. For example in getVRegIdx the parameter is unnamed and unused.

This revision is now accepted and ready to land.Oct 3 2022, 9:37 AM
dp updated this revision to Diff 464758.Oct 3 2022, 11:59 AM
dp marked 9 inline comments as done.

Corrected as suggested by Joe.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
552

I have no idea either, but I do not like repetition. Corrected.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
611

Renamed to hasMandatoryLiteralAt.

651

The Component operand is used in checkVOPDRegConstraints - we have two different components there. See https://reviews.llvm.org/D135084.

This revision was landed with ongoing or failed builds.Oct 7 2022, 5:53 AM
This revision was automatically updated to reflect the committed changes.