[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. #68189

sstipano · 2023-10-04T08:24:55Z

In D151116 it was suggested to have a set of classes to cover every possible case. This does it for bitcast first.

github-actions · 2023-10-04T08:36:55Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jayfoad · 2023-10-06T09:09:34Z

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+         typeInSet(Ty, AllPtrTypes) || Ty.isPointer();
+}
+
+static LegalityPredicate isRegType(unsigned TypeIdx,


Needs a comment explaining what this is. In particular how is it different from isRegisterType and why do we need both (and can we have better names for them)?

isRegisterType check is not sufficient. For example it considers v13s32 as legal which doesn't have corresponding reg class. If this approach is accepted, this would be the first step to replace isRegisterType with isRegType (we can name it differently, of course). The idea is to replace it step by step because it is not as straight forward as replacing all calls to isRegisterType with isRegType.

That sounds like a good plan. Could you add a TODO comment to isRegisterType saying that all uses should be migrated to isRegType?

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector.mir

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll

jayfoad · 2023-10-06T09:25:00Z

There is also some relevant discussion on https://reviews.llvm.org/D148096 which introduced isIllegalRegisterType.

Do we really need both isIllegalRegisterType and isRegType?

sstipano · 2023-10-06T10:13:18Z

There is also some relevant discussion on https://reviews.llvm.org/D148096 which introduced isIllegalRegisterType.

Do we really need both isIllegalRegisterType and isRegType?

I think once we replace all usage of isRegisterType we could do without isIllegalRegisterType.

petar-avramovic · 2023-10-06T10:34:54Z

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+  const LLT V2GlobalPtr = LLT::fixed_vector(2, GlobalPtr);
+  const LLT V4GlobalPtr = LLT::fixed_vector(4, GlobalPtr);
+
+  std::initializer_list<LLT> AllPtrTypes{V2FlatPtr,   V3LocalPtr,  V5LocalPtr,


@jayfoad What should we do with vectors of pointers, list all possible vector sizes that fit in some register class?

I guess so. Can you write test cases for legalizing bitcast, that fail because you don't handle all pointer vector types here yet?

sstipano · 2023-10-24T05:58:49Z

@jayfoad ping

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector.mir

sstipano · 2024-01-31T13:13:08Z

@jayfoad ping

jayfoad · 2024-02-06T14:09:16Z

There is also some relevant discussion on https://reviews.llvm.org/D148096 which introduced isIllegalRegisterType.
Do we really need both isIllegalRegisterType and isRegType?

I think once we replace all usage of isRegisterType we could do without isIllegalRegisterType.

Could you add a TODO comment to isIllegalRegisterType please?

jayfoad

I've just taken a fresh look at this. I do have a couple of concerns:

It introduces yet another list of supported register classes, which will need to be updated whenever we add a new one. But maybe that is unavoidable.
It looks like isRegisterClassType will not be super fast, especially since it has to construct all the pointer types on every call.

jayfoad · 2024-02-06T14:10:34Z

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+  const LLT V2GlobalPtr = LLT::fixed_vector(2, GlobalPtr);
+  const LLT V4GlobalPtr = LLT::fixed_vector(4, GlobalPtr);
+
+  std::initializer_list<LLT> AllPtrTypes{V2FlatPtr,   V3LocalPtr,  V5LocalPtr,


I guess so. Can you write test cases for legalizing bitcast, that fail because you don't handle all pointer vector types here yet?

sstipano · 2024-02-09T16:18:26Z

It looks like isRegisterClassType will not be super fast, especially since it has to construct all the pointer types on every call.
@jayfoad initializer_list makes things tricky. I can make pointer types static to avoid having them constructed on each call. What do you think?

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

jayfoad · 2024-02-12T09:34:48Z

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+
+// Checks whether a type is in the list of legal register types.
+static bool isRegisterClassType(LLT Ty) {
+  if (Ty.isVector() && Ty.getElementType().isPointer())


After #81283 this could be isPointerOrPointerVector.

Which one should go in first?

Doesn't matter.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

jayfoad · 2024-02-12T12:57:50Z

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+
+// Checks whether a type is in the list of legal register types.
+static bool isRegisterClassType(LLT Ty) {
+  if (Ty.isVector() && Ty.getElementType().isPointer())


Doesn't matter.

… types, instead of checking bit width.

jayfoad

Seems reasonable to me, thanks.

llvmbot added backend:AMDGPU llvm:globalisel labels Oct 4, 2023

sstipano changed the title ~~[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types,~~ [AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. Oct 4, 2023

sstefan1 requested a review from arsenm October 4, 2023 08:28

sstipano force-pushed the bitcast_legalizer_rework branch from a7077d7 to 0f19020 Compare October 4, 2023 09:08

sstipano requested review from jayfoad, mbrkusanin and petar-avramovic October 6, 2023 03:46

jayfoad reviewed Oct 6, 2023

View reviewed changes

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector.mir Show resolved Hide resolved

jayfoad reviewed Oct 6, 2023

View reviewed changes

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll Show resolved Hide resolved

petar-avramovic reviewed Oct 6, 2023

View reviewed changes

petar-avramovic reviewed Nov 21, 2023

View reviewed changes

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector.mir Show resolved Hide resolved

sstipano requested a review from jayfoad November 30, 2023 08:41

petar-avramovic mentioned this pull request Jan 26, 2024

GlobalISel: infinite loop trying to legalize G_BUILD_VECTOR #79578

Closed

sstipano force-pushed the bitcast_legalizer_rework branch from 0fd67a8 to f174145 Compare January 29, 2024 19:38

sstipano changed the title ~~[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width.~~ [AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. Closes #79578 Jan 29, 2024

sstipano changed the title ~~[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. Closes #79578~~ [AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. Jan 29, 2024

jayfoad reviewed Feb 6, 2024

View reviewed changes

jayfoad reviewed Feb 9, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp Outdated Show resolved Hide resolved

sstipano force-pushed the bitcast_legalizer_rework branch 3 times, most recently from 44016ee to f2c4211 Compare February 9, 2024 21:10

jayfoad reviewed Feb 12, 2024

View reviewed changes

sstipano force-pushed the bitcast_legalizer_rework branch 3 times, most recently from 78c7f36 to fae364a Compare February 12, 2024 12:34

jayfoad reviewed Feb 12, 2024

View reviewed changes

[AMDGPU][GlobalIsel] Introduce isRegisterClassType to check for legal…

780b970

… types, instead of checking bit width.

sstipano force-pushed the bitcast_legalizer_rework branch from fae364a to 780b970 Compare February 12, 2024 13:12

sstipano requested a review from jayfoad February 12, 2024 13:13

jayfoad approved these changes Feb 12, 2024

View reviewed changes

sstipano merged commit 785eddd into llvm:main Feb 13, 2024

sstipano mentioned this pull request Aug 1, 2024

[AMDGPU][GlobalIsel] Use isRegisterClassType for G_FREEZE and G_IMPLICIT_DEF #101331

Merged

[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. #68189

[AMDGPU][GlobalIsel] Introduce isRegType to check for legal types, instead of checking bit width. #68189

Uh oh!

Conversation

sstipano commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayfoad commented Oct 6, 2023

Uh oh!

sstipano commented Oct 6, 2023

Uh oh!

petar-avramovic Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstipano commented Oct 24, 2023

Uh oh!

Uh oh!

sstipano commented Jan 31, 2024

Uh oh!

jayfoad commented Feb 6, 2024

Uh oh!

jayfoad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstipano commented Feb 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayfoad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sstipano commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

petar-avramovic Oct 6, 2023 •

edited

Loading