This is an archive of the discontinued LLVM Phabricator instance.

Differential D60457

[CodeGen] Fixed de-optimization of legalize subvector extract
ClosedPublic

Authored by tpr on Apr 9 2019, 5:49 AM.

Details

Reviewers
hakzsam
arsenm
efriedma
Commits
rZORG3fec75d017ad: [CodeGen] Fixed de-optimization of legalize subvector extract
rG3fec75d017ad: [CodeGen] Fixed de-optimization of legalize subvector extract
rGe3cbdaf1b5e7: [CodeGen] Fixed de-optimization of legalize subvector extract
rL360942: [CodeGen] Fixed de-optimization of legalize subvector extract
Summary

The recent introduction of v3i32 etc as an MVT, and its use in AMDGPU
3-dword memory instructions, caused a de-optimization problem for code
with such a load that then bitcasts via vector of i8, because v12i8 is
not an MVT so it legalizes the bitcast by widening it.

This commit adds the ability to widen a bitcast using extract_subvector
on the result, so the value does not need to go via memory.

Change-Id: Ie4abb7760547e54a2445961992eafc78e80d4b64

Diff Detail

Event Timeline

tpr created this revision.Apr 9 2019, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 5:49 AM
Herald added subscribers: llvm-commits, nhaehnle, jvesely. · View Herald Transcript
Harbormaster completed remote builds in B30227: Diff 194299.Apr 9 2019, 5:50 AM
tpr mentioned this in D58902: [AMDGPU] Support for v3i32/v3f32.Apr 9 2019, 5:56 AM
tpr added reviewers: hakzsam, arsenm, efriedma.Apr 9 2019, 6:10 AM
Herald added a subscriber: wdng. · View Herald TranscriptApr 9 2019, 6:10 AM
hakzsam added a comment.Apr 9 2019, 7:26 AM

This fixes the SI regression with RADV.
Thanks a lot Tim.

efriedma added inline comments.Apr 9 2019, 12:35 PM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
4066

So if I'm following this correctly, this takes a cast like <12 x i8> -> <3 x i32>, and turns it into <16 x i8> -> <4 x i32>? That makes sense, but please add a comment describing it.

arsenm added a comment.Apr 10 2019, 3:38 AM

Can you reduce the test further?

test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

I wouldn’t trust this to check this, a generated check would be better

tpr updated this revision to Diff 195527.Apr 17 2019, 3:05 AM

V2: Addressed review comments.

Harbormaster completed remote builds in B30674: Diff 195527.Apr 17 2019, 3:05 AM
tpr updated this revision to Diff 195528.Apr 17 2019, 3:11 AM

V3: Further reduced test case.

Harbormaster completed remote builds in B30675: Diff 195528.Apr 17 2019, 3:12 AM
tpr marked 3 inline comments as done.Apr 17 2019, 3:12 AM
tpr added inline comments.
test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

Not really sure what you're suggesting, but I hope this is better.

hakzsam added a comment.Apr 26 2019, 12:40 AM

ping?
SI is still broken without this patch.

nhaehnle added inline comments.May 7 2019, 1:27 AM
test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

Maybe you can use util/update_llc_test_checks.py?

nhaehnle mentioned this in D61553: AMDGPU: Fix ds_{read,write}2_b64 on SI/gfx6.May 7 2019, 4:27 AM
tpr marked 2 inline comments as done.May 8 2019, 2:31 AM

Hi Samuel. Sorry for the delay; I kind of lost track of this change.

Question for Nicolai below.

test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

You mean have a check line for each line of IR output in the function? Do you think that would be better than the negative check for storing to stack?

nhaehnle added inline comments.May 13 2019, 5:19 AM
test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

Yes, I do think so. Having the auto-generated assertions means that we catch other things going wrong, and it's easy enough to update them for benign changes.

I realize that you actually need update_mir_test_checks in this case due to the -stop-after, and the script is sensitive to the fact that there's no space between the < and the %s.

tpr updated this revision to Diff 199446.May 14 2019, 8:04 AM

V4: update_mir_test_checks the test.

Harbormaster completed remote builds in B31881: Diff 199446.May 14 2019, 8:06 AM
tpr marked 2 inline comments as done.May 14 2019, 8:08 AM
tpr added inline comments.
test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll
5

Thanks Nicolai. Now done.

tpr marked an inline comment as done.May 15 2019, 10:35 AM
tpr added a comment.May 16 2019, 10:19 AM

Is someone now able to approve this? Eli?

efriedma accepted this revision.May 16 2019, 12:16 PM

LGTM, assuming the review comments about the testcase are resolved.

This revision is now accepted and ready to land.May 16 2019, 12:16 PM
Closed by commit rL360942: [CodeGen] Fixed de-optimization of legalize subvector extract (authored by tpr). · Explain WhyMay 16 2019, 2:46 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
CodeGen/
SelectionDAG/
18 lines
test/
CodeGen/
AMDGPU/
19 lines

Diff 195528

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

Show First 20 Lines • Show All 4,057 Lines • ▼ Show 20 Lines if (InWidenSize % Size == 0 && !VT.isVector() && VT != MVT::x86mmx) {
if (TLI.isTypeLegal(NewVT)) { if (TLI.isTypeLegal(NewVT)) {
SDValue BitOp = DAG.getNode(ISD::BITCAST, dl, NewVT, InOp); SDValue BitOp = DAG.getNode(ISD::BITCAST, dl, NewVT, InOp);
return DAG.getNode( return DAG.getNode(
ISD::EXTRACT_VECTOR_ELT, dl, VT, BitOp, ISD::EXTRACT_VECTOR_ELT, dl, VT, BitOp,
DAG.getConstant(0, dl, TLI.getVectorIdxTy(DAG.getDataLayout()))); DAG.getConstant(0, dl, TLI.getVectorIdxTy(DAG.getDataLayout())));
} }
} }
// Handle a case like bitcast v12i8 -> v3i32. Normally that would get widened
efriedmaUnsubmitted
Done
ReplyInline Actions

So if I'm following this correctly, this takes a cast like <12 x i8> -> <3 x i32>, and turns it into <16 x i8> -> <4 x i32>? That makes sense, but please add a comment describing it.

efriedma: So if I'm following this correctly, this takes a cast like `<12 x i8>` -> `<3 x i32>`, and…
// to v16i8 -> v4i32, but for a target where v3i32 is legal but v12i8 is not,
// we end up here. Handling the case here with EXTRACT_SUBVECTOR avoids
// having to copy via memory.
if (VT.isVector()) {
EVT EltVT = VT.getVectorElementType();
unsigned EltSize = EltVT.getSizeInBits();
if (InWidenSize % EltSize == 0) {
unsigned NewNumElts = InWidenSize / EltSize;
EVT NewVT = EVT::getVectorVT(*DAG.getContext(), EltVT, NewNumElts);
if (TLI.isTypeLegal(NewVT)) {
SDValue BitOp = DAG.getNode(ISD::BITCAST, dl, NewVT, InOp);
return DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VT, BitOp,
DAG.getConstant(0, dl, TLI.getVectorIdxTy(DAG.getDataLayout())));
}
}
}
return CreateStackStoreLoad(InOp, VT); return CreateStackStoreLoad(InOp, VT);
} }
SDValue DAGTypeLegalizer::WidenVecOp_CONCAT_VECTORS(SDNode *N) { SDValue DAGTypeLegalizer::WidenVecOp_CONCAT_VECTORS(SDNode *N) {
EVT VT = N->getValueType(0); EVT VT = N->getValueType(0);
EVT EltVT = VT.getVectorElementType(); EVT EltVT = VT.getVectorElementType();
EVT InVT = N->getOperand(0).getValueType(); EVT InVT = N->getOperand(0).getValueType();
SDLoc dl(N); SDLoc dl(N);
▲ Show 20 Lines • Show All 735 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/extract_subvector_vec4_vec3.ll

  • This file was added.
; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx900 <%s -stop-after=amdgpu-isel | FileCheck -check-prefix=GCN %s
; GCN-LABEL: body:
; GCN-NOT: BUFFER_STORE{{.*}}store{{.*}}into{{.*}}stack
; GCN: S_ENDPGM
arsenmUnsubmitted
Done
ReplyInline Actions

I wouldn’t trust this to check this, a generated check would be better

arsenm: I wouldn’t trust this to check this, a generated check would be better
tprAuthorUnsubmitted
Done
ReplyInline Actions

Not really sure what you're suggesting, but I hope this is better.

tpr: Not really sure what you're suggesting, but I hope this is better.
nhaehnleUnsubmitted
Not Done
ReplyInline Actions

Maybe you can use util/update_llc_test_checks.py?

nhaehnle: Maybe you can use `util/update_llc_test_checks.py`?
tprAuthorUnsubmitted
Done
ReplyInline Actions

You mean have a check line for each line of IR output in the function? Do you think that would be better than the negative check for storing to stack?

tpr: You mean have a check line for each line of IR output in the function? Do you think that would…
nhaehnleUnsubmitted
Done
ReplyInline Actions

Yes, I do think so. Having the auto-generated assertions means that we catch other things going wrong, and it's easy enough to update them for benign changes.

I realize that you actually need update_mir_test_checks in this case due to the -stop-after, and the script is sensitive to the fact that there's no space between the < and the %s.

nhaehnle: Yes, I do think so. Having the auto-generated assertions means that we catch other things going…
tprAuthorUnsubmitted
Done
ReplyInline Actions

Thanks Nicolai. Now done.

tpr: Thanks Nicolai. Now done.
define amdgpu_hs void @main([0 x i8] addrspace(6)* inreg %arg) {
main_body:
%tmp25 = call <4 x float> @llvm.amdgcn.raw.buffer.load.v4f32(<4 x i32> undef, i32 undef, i32 0, i32 0)
%tmp27 = bitcast <4 x float> %tmp25 to <16 x i8>
%tmp28 = shufflevector <16 x i8> %tmp27, <16 x i8> undef, <12 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 10, i32 11>
%tmp29 = bitcast <12 x i8> %tmp28 to <3 x i32>
call void @llvm.amdgcn.raw.buffer.store.v3i32(<3 x i32> %tmp29, <4 x i32> undef, i32 undef, i32 0, i32 0) #3
ret void
}
declare void @llvm.amdgcn.raw.buffer.store.v3i32(<3 x i32>, <4 x i32>, i32, i32, i32 immarg)
declare <4 x float> @llvm.amdgcn.raw.buffer.load.v4f32(<4 x i32>, i32, i32, i32 immarg)