This is an archive of the discontinued LLVM Phabricator instance.

Differential D69915

[AMDGPU] Fix bug introduced in 47a5c36b37f0
ClosedPublic

Authored by dfukalov on Nov 6 2019, 11:07 AM.

Details

Reviewers
foad
arsenm
Commits
rG6e8251046b96: [AMDGPU] Fix bug introduced in 47a5c36b37f0
Summary

[AMDGPU] Fix bug introduced in 47a5c36b37f0

Diff Detail

Event Timeline

dfukalov created this revision.Nov 6 2019, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 11:07 AM
Herald added subscribers: hiraditya, t-tye, tpr and 7 others. · View Herald Transcript
Harbormaster completed remote builds in B40588: Diff 228106.Nov 6 2019, 11:07 AM
arsenm added inline comments.Nov 6 2019, 11:08 AM
llvm/test/Analysis/CostModel/AMDGPU/shufflevector.ll
55

Should still be checking the returned cost

dfukalov edited the summary of this revision. (Show Details)Nov 6 2019, 11:11 AM
dfukalov updated this revision to Diff 228113.Nov 6 2019, 11:38 AM

Address comment

dfukalov marked an inline comment as done.Nov 6 2019, 11:38 AM
Harbormaster completed remote builds in B40589: Diff 228113.Nov 6 2019, 11:43 AM
arsenm added inline comments.Nov 6 2019, 11:45 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
741

I don't see how this works? Shouldn't this be returning instead of falling through?

dfukalov updated this revision to Diff 228119.Nov 6 2019, 12:05 PM

fix typo

dfukalov marked 2 inline comments as done.Nov 6 2019, 12:07 PM
dfukalov added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
741

You're right, thanks!

Harbormaster completed remote builds in B40590: Diff 228119.Nov 6 2019, 12:11 PM
arsenm accepted this revision.Nov 6 2019, 12:12 PM

LGTM

This revision is now accepted and ready to land.Nov 6 2019, 12:12 PM
arsenm added a comment.Nov 6 2019, 4:14 PM

Commit message could uses work though, include revision in the long description, not the first line

Closed by commit rG6e8251046b96: [AMDGPU] Fix bug introduced in 47a5c36b37f0 (authored by dfukalov). · Explain WhyNov 7 2019, 12:50 AM
This revision was automatically updated to reflect the committed changes.
dfukalov marked an inline comment as done.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
2 lines
test/
Analysis/
CostModel/
AMDGPU/
19 lines

Diff 228185

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

Show First 20 Lines • Show All 732 Lines • ▼ Show 20 Lines case Instruction::ShuffleVector: {
Type *SrcTy = Shuffle->getOperand(0)->getType(); Type *SrcTy = Shuffle->getOperand(0)->getType();
// TODO: Identify and add costs for insert subvector, etc. // TODO: Identify and add costs for insert subvector, etc.
int SubIndex; int SubIndex;
if (Shuffle->isExtractSubvectorMask(SubIndex)) if (Shuffle->isExtractSubvectorMask(SubIndex))
return getShuffleCost(TTI::SK_ExtractSubvector, SrcTy, SubIndex, Ty); return getShuffleCost(TTI::SK_ExtractSubvector, SrcTy, SubIndex, Ty);
if (Shuffle->changesLength()) if (Shuffle->changesLength())
return -1; return BaseT::getUserCost(U, Operands);
arsenmUnsubmitted
Done
ReplyInline Actions

I don't see how this works? Shouldn't this be returning instead of falling through?

arsenm: I don't see how this works? Shouldn't this be returning instead of falling through?
dfukalovAuthorUnsubmitted
Done
ReplyInline Actions

You're right, thanks!

dfukalov: You're right, thanks!
if (Shuffle->isIdentity()) if (Shuffle->isIdentity())
return 0; return 0;
if (Shuffle->isReverse()) if (Shuffle->isReverse())
return getShuffleCost(TTI::SK_Reverse, Ty, 0, nullptr); return getShuffleCost(TTI::SK_Reverse, Ty, 0, nullptr);
if (Shuffle->isSelect()) if (Shuffle->isSelect())
▲ Show 20 Lines • Show All 161 Lines • Show Last 20 Lines

llvm/test/Analysis/CostModel/AMDGPU/shufflevector.ll

; RUN: opt -cost-model -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 %s | FileCheck -check-prefixes=GFX9,GCN %s ; RUN: opt -cost-model -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 %s | FileCheck -check-prefixes=GFX9,GCN,TPT %s
; RUN: opt -cost-model -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=fiji %s | FileCheck -check-prefixes=VI,GCN %s ; RUN: opt -cost-model -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=fiji %s | FileCheck -check-prefixes=VI,GCN,TPT %s
; RUN: opt -cost-model -cost-kind=code-size -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 %s | FileCheck -check-prefixes=GFX9,GCN %s ; RUN: opt -cost-model -cost-kind=code-size -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx900 %s | FileCheck -check-prefixes=GFX9,GCN,CS %s
; RUN: opt -cost-model -cost-kind=code-size -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=fiji %s | FileCheck -check-prefixes=VI,GCN %s ; RUN: opt -cost-model -cost-kind=code-size -analyze -mtriple=amdgcn-unknown-amdhsa -mcpu=fiji %s | FileCheck -check-prefixes=VI,GCN,CS %s
; GCN-LABEL: 'shufflevector_00_v2i16' ; GCN-LABEL: 'shufflevector_00_v2i16'
; GFX9: estimated cost of 0 for {{.*}} shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer ; GFX9: estimated cost of 0 for {{.*}} shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer
; VI: estimated cost of 1 for {{.*}} shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer ; VI: estimated cost of 1 for {{.*}} shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer
define amdgpu_kernel void @shufflevector_00_v2i16(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %vaddr) { define amdgpu_kernel void @shufflevector_00_v2i16(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %vaddr) {
%vec = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr %vec = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr
%shuf = shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer %shuf = shufflevector <2 x i16> %vec, <2 x i16> undef, <2 x i32> zeroinitializer
store <2 x i16> %shuf, <2 x i16> addrspace(1)* %out store <2 x i16> %shuf, <2 x i16> addrspace(1)* %out
Show All 33 Lines
; GCN: estimated cost of 2 for {{.*}} shufflevector <2 x i16> %vec0, <2 x i16> %vec1, <2 x i32> <i32 0, i32 2> ; GCN: estimated cost of 2 for {{.*}} shufflevector <2 x i16> %vec0, <2 x i16> %vec1, <2 x i32> <i32 0, i32 2>
define amdgpu_kernel void @shufflevector_02_v2i16(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %vaddr0, <2 x i16> addrspace(1)* %vaddr1) { define amdgpu_kernel void @shufflevector_02_v2i16(<2 x i16> addrspace(1)* %out, <2 x i16> addrspace(1)* %vaddr0, <2 x i16> addrspace(1)* %vaddr1) {
%vec0 = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr0 %vec0 = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr0
%vec1 = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr1 %vec1 = load <2 x i16>, <2 x i16> addrspace(1)* %vaddr1
%shuf = shufflevector <2 x i16> %vec0, <2 x i16> %vec1, <2 x i32> <i32 0, i32 2> %shuf = shufflevector <2 x i16> %vec0, <2 x i16> %vec1, <2 x i32> <i32 0, i32 2>
store <2 x i16> %shuf, <2 x i16> addrspace(1)* %out store <2 x i16> %shuf, <2 x i16> addrspace(1)* %out
ret void ret void
} }
; GCN-LABEL: 'shufflevector_xxx'
arsenmUnsubmitted
Done
ReplyInline Actions

Should still be checking the returned cost

arsenm: Should still be checking the returned cost
; TPT: Unknown cost for {{.*}} shufflevector <2 x i8> %vec, <2 x i8> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
; CS: estimated cost of 1 for {{.*}} shufflevector <2 x i8> %vec, <2 x i8> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
; Should not assert
define amdgpu_kernel void @shufflevector_xxx(<4 x i8> addrspace(1)* %out, <2 x i8> addrspace(1)* %vaddr) {
%vec = load <2 x i8>, <2 x i8> addrspace(1)* %vaddr
%shuf = shufflevector <2 x i8> %vec, <2 x i8> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
store <4 x i8> %shuf, <4 x i8> addrspace(1)* %out
ret void
}