This is an archive of the discontinued LLVM Phabricator instance.

Differential D49836

AMDGPU: Remove broken ternary i16 patterns
ClosedPublic

Authored by jvesely on Jul 25 2018, 11:55 PM.

Details

Reviewers
arsenm
tstellar
Commits
rG466ccce944b2: Merging r339190: --------------------------------------------------------------…
rG7b2c98ab5991: AMDGPU: Remove broken i16 ternary patterns
rL339235: Merging r339190:
rL339190: AMDGPU: Remove broken i16 ternary patterns
Summary

Fixup test to check for GCN prefix
These patterns always zero extend the result even though it might need sign extension.
This has been broken since the addition of i16 support.
It has popped up in mad_sat(char) test since min(max()) combination is turned into v_med3, resulting in the following (incorrect) sequence:

v_mad_i16 v2, v10, v9, v11
v_med3_i32 v2, v2, v8, v7

Fixes mad_sat(char) piglit on VI.

Diff Detail

Repository
rL LLVM

Event Timeline

jvesely created this revision.Jul 25 2018, 11:55 PM
Herald added subscribers: t-tye, tpr, dstuttard and 4 others. · View Herald TranscriptJul 25 2018, 11:55 PM
arsenm added inline comments.Jul 26 2018, 1:46 AM
lib/Target/AMDGPU/VOP3Instructions.td
469–474

These can be fixed, they don't need to be dropped.

It's also more complicated because since gfx9 there is a control bit for whether the high 16-bits are zeroed or preserved which we haven't implemented

jvesely added inline comments.Jul 26 2018, 10:00 AM
lib/Target/AMDGPU/VOP3Instructions.td
469–474

There is no reason to preserve and fix them since they'd need to be the same instructions which end up being used anyway. There is nothing to be gained from having the extension part in the pattern.

the gfx9 part does not apply you can drop zero_extend if v_mad_i16 clears those bits, but that should not be handled in this pattern.

jvesely added inline comments.Jul 26 2018, 12:10 PM
lib/Target/AMDGPU/VOP3Instructions.td
469–474

In fact including the extension op here would be damaging. Leaving it outside exposes it to other patterns that could replace or eliminate it.

jvesely added a comment.Aug 3 2018, 10:48 AM

ping.
Can we just have the fix in, and worry about optimizing i16 extends later?

arsenm accepted this revision.Aug 7 2018, 12:56 AM

LGTM

test/CodeGen/AMDGPU/mad_uint24.ll
192–194

instnamer on these

This revision is now accepted and ready to land.Aug 7 2018, 12:56 AM
jvesely updated this revision to Diff 159595.Aug 7 2018, 2:14 PM

rename numbered operations

jvesely marked an inline comment as done.Aug 7 2018, 2:15 PM
Closed by commit rL339190: AMDGPU: Remove broken i16 ternary patterns (authored by jvesely). · Explain WhyAug 7 2018, 2:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Target/
AMDGPU/
11 lines
test/
CodeGen/
AMDGPU/
93 lines

Diff 157426

lib/Target/AMDGPU/VOP3Instructions.td

Context not available.
(inst i16:$src0, i16:$src1, i16:$src2, (i1 0)) (inst i16:$src0, i16:$src1, i16:$src2, (i1 0))
>; >;
def : GCNPat<
(i32 (op3 (op2 (op1 i16:$src0, i16:$src1), i16:$src2))),
(inst i16:$src0, i16:$src1, i16:$src2, (i1 0))
>;
def : GCNPat<
(i64 (op3 (op2 (op1 i16:$src0, i16:$src1), i16:$src2))),
(REG_SEQUENCE VReg_64,
(inst i16:$src0, i16:$src1, i16:$src2, (i1 0)), sub0,
(V_MOV_B32_e32 (i32 0)), sub1)
>;
arsenmUnsubmitted
Not Done
ReplyInline Actions

These can be fixed, they don't need to be dropped.

It's also more complicated because since gfx9 there is a control bit for whether the high 16-bits are zeroed or preserved which we haven't implemented

arsenm: These can be fixed, they don't need to be dropped. It's also more complicated because since…
jveselyAuthorUnsubmitted
Not Done
ReplyInline Actions

There is no reason to preserve and fix them since they'd need to be the same instructions which end up being used anyway. There is nothing to be gained from having the extension part in the pattern.

the gfx9 part does not apply you can drop zero_extend if v_mad_i16 clears those bits, but that should not be handled in this pattern.

jvesely: There is no reason to preserve and fix them since they'd need to be the same instructions which…
jveselyAuthorUnsubmitted
Not Done
ReplyInline Actions

In fact including the extension op here would be damaging. Leaving it outside exposes it to other patterns that could replace or eliminate it.

jvesely: In fact including the extension op here would be damaging. Leaving it outside exposes it to…
} }
defm: Ternary_i16_Pats<mul, add, V_MAD_U16, zext>; defm: Ternary_i16_Pats<mul, add, V_MAD_U16, zext>;
Context not available.

test/CodeGen/AMDGPU/mad_uint24.ll

; RUN: llc < %s -march=r600 -mcpu=redwood | FileCheck %s --check-prefix=EG --check-prefix=FUNC ; RUN: llc < %s -march=r600 -mcpu=redwood | FileCheck %s --check-prefix=EG --check-prefix=FUNC
; RUN: llc < %s -march=r600 -mcpu=cayman | FileCheck %s --check-prefix=EG --check-prefix=FUNC ; RUN: llc < %s -march=r600 -mcpu=cayman | FileCheck %s --check-prefix=EG --check-prefix=FUNC
; RUN: llc < %s -march=amdgcn -verify-machineinstrs | FileCheck %s --check-prefix=SI --check-prefix=FUNC ; RUN: llc < %s -march=amdgcn -verify-machineinstrs | FileCheck %s --check-prefix=SI --check-prefix=FUNC --check-prefix=GCN
; RUN: llc < %s -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs | FileCheck %s --check-prefix=VI --check-prefix=FUNC ; RUN: llc < %s -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs | FileCheck %s --check-prefix=VI --check-prefix=FUNC --check-prefix=GCN
; RUN: llc < %s -march=amdgcn -mcpu=fiji -mattr=-flat-for-global -verify-machineinstrs | FileCheck %s --check-prefix=VI --check-prefix=FUNC ; RUN: llc < %s -march=amdgcn -mcpu=fiji -mattr=-flat-for-global -verify-machineinstrs | FileCheck %s --check-prefix=VI --check-prefix=FUNC --check-prefix=GCN
declare i32 @llvm.r600.read.tidig.x() nounwind readnone declare i32 @llvm.r600.read.tidig.x() nounwind readnone
Context not available.
store i32 %tmp16, i32 addrspace(1)* %arg store i32 %tmp16, i32 addrspace(1)* %arg
ret void ret void
} }
; FUNC-LABEL: {{^}}i8_mad_sat_16:
; EG: MULADD_UINT24 {{[* ]*}}T{{[0-9]}}.[[MAD_CHAN:[XYZW]]]
; The result must be sign-extended
; EG: BFE_INT {{[* ]*}}T{{[0-9]\.[XYZW]}}, PV.[[MAD_CHAN]], 0.0, literal.x
; EG: 8
; SI: v_mad_u32_u24 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; VI: v_mad_u16 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; GCN: v_bfe_i32 [[EXT:v[0-9]]], [[MAD]], 0, 16
; GCN: v_med3_i32 v{{[0-9]}}, [[EXT]],
define amdgpu_kernel void @i8_mad_sat_16(i8 addrspace(1)* %out, i8 addrspace(1)* %in0, i8 addrspace(1)* %in1, i8 addrspace(1)* %in2, i64 addrspace(5)* %idx) {
entry:
%retval.0.i = load i64, i64 addrspace(5)* %idx
%arrayidx = getelementptr inbounds i8, i8 addrspace(1)* %in0, i64 %retval.0.i
%arrayidx2 = getelementptr inbounds i8, i8 addrspace(1)* %in1, i64 %retval.0.i
%arrayidx4 = getelementptr inbounds i8, i8 addrspace(1)* %in2, i64 %retval.0.i
%l1 = load i8, i8 addrspace(1)* %arrayidx, align 1
%l2 = load i8, i8 addrspace(1)* %arrayidx2, align 1
%l3 = load i8, i8 addrspace(1)* %arrayidx4, align 1
%conv1.i = sext i8 %l1 to i16
%conv3.i = sext i8 %l2 to i16
%conv5.i = sext i8 %l3 to i16
%mul.i.i.i = mul nsw i16 %conv3.i, %conv1.i
%add.i.i = add i16 %mul.i.i.i, %conv5.i
%c4 = icmp sgt i16 %add.i.i, -128
%cond.i.i = select i1 %c4, i16 %add.i.i, i16 -128
%c5 = icmp slt i16 %cond.i.i, 127
%cond13.i.i = select i1 %c5, i16 %cond.i.i, i16 127
%conv8.i = trunc i16 %cond13.i.i to i8
%arrayidx7 = getelementptr inbounds i8, i8 addrspace(1)* %out, i64 %retval.0.i
store i8 %conv8.i, i8 addrspace(1)* %arrayidx7, align 1
ret void
}
; FUNC-LABEL: {{^}}i8_mad_32:
; EG: MULADD_UINT24 {{[* ]*}}T{{[0-9]}}.[[MAD_CHAN:[XYZW]]]
; The result must be sign-extended
; EG: BFE_INT {{[* ]*}}T{{[0-9]\.[XYZW]}}, PV.[[MAD_CHAN]], 0.0, literal.x
; EG: 8
; SI: v_mad_u32_u24 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; VI: v_mad_u16 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; GCN: v_bfe_i32 [[EXT:v[0-9]]], [[MAD]], 0, 16
define amdgpu_kernel void @i8_mad_32(i32 addrspace(1)* %out, i8 addrspace(1)* %a, i8 addrspace(1)* %b, i8 addrspace(1)* %c, i64 addrspace(5)* %idx) {
entry:
%retval.0.i = load i64, i64 addrspace(5)* %idx
%arrayidx = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 %retval.0.i
%arrayidx2 = getelementptr inbounds i8, i8 addrspace(1)* %b, i64 %retval.0.i
%arrayidx4 = getelementptr inbounds i8, i8 addrspace(1)* %c, i64 %retval.0.i
%la = load i8, i8 addrspace(1)* %arrayidx, align 1
%lb = load i8, i8 addrspace(1)* %arrayidx2, align 1
%lc = load i8, i8 addrspace(1)* %arrayidx4, align 1
%0 = sext i8 %la to i16
%1 = sext i8 %lb to i16
%2 = sext i8 %lc to i16
arsenmUnsubmitted
Done
ReplyInline Actions

instnamer on these

arsenm: instnamer on these
%3 = mul i16 %0, %1
%4 = add i16 %3, %2
%5 = sext i16 %4 to i32
store i32 %5, i32 addrspace(1)* %out
ret void
}
; FUNC-LABEL: {{^}}i8_mad_64:
; EG: MULADD_UINT24 {{[* ]*}}T{{[0-9]}}.[[MAD_CHAN:[XYZW]]]
; The result must be sign-extended
; EG: BFE_INT {{[* ]*}}T{{[0-9]\.[XYZW]}}, PV.[[MAD_CHAN]], 0.0, literal.x
; EG: 8
; SI: v_mad_u32_u24 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; VI: v_mad_u16 [[MAD:v[0-9]]], {{[sv][0-9], [sv][0-9]}}
; GCN: v_bfe_i32 [[EXT:v[0-9]]], [[MAD]], 0, 16
define amdgpu_kernel void @i8_mad_64(i64 addrspace(1)* %out, i8 addrspace(1)* %a, i8 addrspace(1)* %b, i8 addrspace(1)* %c, i64 addrspace(5)* %idx) {
entry:
%retval.0.i = load i64, i64 addrspace(5)* %idx
%arrayidx = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 %retval.0.i
%arrayidx2 = getelementptr inbounds i8, i8 addrspace(1)* %b, i64 %retval.0.i
%arrayidx4 = getelementptr inbounds i8, i8 addrspace(1)* %c, i64 %retval.0.i
%la = load i8, i8 addrspace(1)* %arrayidx, align 1
%lb = load i8, i8 addrspace(1)* %arrayidx2, align 1
%lc = load i8, i8 addrspace(1)* %arrayidx4, align 1
%0 = sext i8 %la to i16
%1 = sext i8 %lb to i16
%2 = sext i8 %lc to i16
%3 = mul i16 %0, %1
%4 = add i16 %3, %2
%5 = sext i16 %4 to i64
store i64 %5, i64 addrspace(1)* %out
ret void
}
Context not available.