This is an archive of the discontinued LLVM Phabricator instance.

Differential D21362

AMDGPU: Added v_mad instruction.
ClosedPublic

Authored by wdng on Jun 14 2016, 8:25 PM.

Details

Reviewers
• tstellarAMD
arsenm
Commits
rGab3d91b8f1d3: AMDGPU: Add v_mad 16-bit instructions definition.
rL272919: AMDGPU: Add v_mad 16-bit instructions definition.
Summary

Added v_mad_16 instruction definition.

Diff Detail

Repository
rL LLVM

Event Timeline

wdng updated this revision to Diff 60788.Jun 14 2016, 8:25 PM
wdng retitled this revision from to Added v_mad instruction..
wdng updated this object.
wdng added reviewers: arsenm, • tstellarAMD.
wdng set the repository for this revision to rL LLVM.
wdng added a project: Restricted Project.
Herald added a subscriber: arsenm. · View Herald TranscriptJun 14 2016, 8:25 PM
• tstellarAMD edited edge metadata.EditedJun 14 2016, 8:28 PM

The commit subject should be be prefixed with AMDGPU:

lib/Target/AMDGPU/VIInstructions.td
78–83

Can we also add a pattern and a test with this patch?

wdng retitled this revision from Added v_mad instruction. to AMDGPU: Added v_mad instruction..Jun 14 2016, 8:29 PM
wdng edited edge metadata.
wdng updated this revision to Diff 60789.EditedJun 14 2016, 8:34 PM

This pattern has been tested in LIT test mad_uint24.ll, which is a part of fp16 implementation.

Herald added a subscriber: kzhuravl. · View Herald TranscriptJun 14 2016, 8:34 PM
arsenm added inline comments.Jun 14 2016, 8:36 PM
lib/Target/AMDGPU/VIInstructions.td
82

The unsigned version should be also added

281–284

This should be left for the i16 patch

wdng added a comment.Jun 14 2016, 8:37 PM

Pattern is added for LIT test mad_uint24.ll

wdng added inline comments.Jun 14 2016, 8:38 PM
lib/Target/AMDGPU/VIInstructions.td
281–284

Yes, we may not need the pattern. For this patch, we may only need to add the instruction definition for the time being.

arsenm added inline comments.Jun 14 2016, 8:49 PM
lib/Target/AMDGPU/VIInstructions.td
281–284

The pattern will do nothing here, so yes only the definitions

286

This is also trying to use half mul for integer mul

wdng updated this revision to Diff 60853.Jun 15 2016, 10:42 AM
wdng updated this object.
wdng marked 4 inline comments as done.

Added v_mad_u16 instruction definition.

wdng added inline comments.Jun 15 2016, 10:59 AM
lib/Target/AMDGPU/VIInstructions.td
86

Could you please say in more details? Thanks!

arsenm edited edge metadata.Jun 15 2016, 6:35 PM

The v_mad_f16 instruction is also missing

lib/Target/AMDGPU/VIInstructions.td
82–83

These aren't the opcode values I see in the manual

wdng updated this revision to Diff 60943.Jun 15 2016, 6:59 PM
wdng edited edge metadata.
wdng marked an inline comment as done.
  1. Modified opcode for v_mad_u16, v_mad_i16 to make it consistant with the GCN manual.
  2. Added v_mad_f16 instruction definition.
arsenm accepted this revision.Jun 15 2016, 7:04 PM
arsenm edited edge metadata.

With the capitalization fix LGTM

lib/Target/AMDGPU/VIInstructions.td
82

All of the other opcodes use lower case letters in the hex.

This revision is now accepted and ready to land.Jun 15 2016, 7:04 PM
Closed by commit rL272919: AMDGPU: Add v_mad 16-bit instructions definition. (authored by wdng). · Explain WhyJun 16 2016, 9:56 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Target/
AMDGPU/
1 line
6 lines

Diff 60788

lib/Target/AMDGPU/SIInstrInfo.td

Context not available.
def VOP_F16_F16_F16 : VOPProfile <[f16, f16, f16, untyped]>; def VOP_F16_F16_F16 : VOPProfile <[f16, f16, f16, untyped]>;
def VOP_F16_F16_I16 : VOPProfile <[f16, f16, i32, untyped]>; def VOP_F16_F16_I16 : VOPProfile <[f16, f16, i32, untyped]>;
def VOP_I16_I16_I16 : VOPProfile <[i32, i32, i32, untyped]>; def VOP_I16_I16_I16 : VOPProfile <[i32, i32, i32, untyped]>;
def VOP_I16_I16_I16_I16 : VOPProfile <[i32, i32, i32, i32, untyped]>;
def VOP_NONE : VOPProfile <[untyped, untyped, untyped, untyped]>; def VOP_NONE : VOPProfile <[untyped, untyped, untyped, untyped]>;
Context not available.

lib/Target/AMDGPU/VIInstructions.td

Context not available.
} // End isCommutable = 1 } // End isCommutable = 1
defm V_LDEXP_F16 : VOP2Inst <vop2<0,0x33>, "v_ldexp_f16", VOP_F16_F16_I16>; defm V_LDEXP_F16 : VOP2Inst <vop2<0,0x33>, "v_ldexp_f16", VOP_F16_F16_I16>;
//===----------------------------------------------------------------------===//
// VOP3 Instructions
//===----------------------------------------------------------------------===//
let isCommutable = 1 in {
defm V_MAD_I16 : VOP3Inst <vop3<0, 0x34>, "v_mad_i16", VOP_I16_I16_I16_I16>;
arsenmUnsubmitted
Done
ReplyInline Actions

The unsigned version should be also added

arsenm: The unsigned version should be also added
arsenmUnsubmitted
Not Done
ReplyInline Actions

All of the other opcodes use lower case letters in the hex.

arsenm: All of the other opcodes use lower case letters in the hex.
}
tstellarAMDUnsubmitted
Not Done
ReplyInline Actions

Can we also add a pattern and a test with this patch?

tstellarAMD: Can we also add a pattern and a test with this patch?
arsenmUnsubmitted
Done
ReplyInline Actions

These aren't the opcode values I see in the manual

arsenm: These aren't the opcode values I see in the manual
} // let DisableSIDecoder = 1 } // let DisableSIDecoder = 1
// Aliases to simplify matching of floating-point instructions that // Aliases to simplify matching of floating-point instructions that
Context not available.
arsenmUnsubmitted
Done
ReplyInline Actions

This should be left for the i16 patch

arsenm: This should be left for the i16 patch
wdngAuthorUnsubmitted
Done
ReplyInline Actions

Yes, we may not need the pattern. For this patch, we may only need to add the instruction definition for the time being.

wdng: Yes, we may not need the pattern. For this patch, we may only need to add the instruction…
arsenmUnsubmitted
Done
ReplyInline Actions

The pattern will do nothing here, so yes only the definitions

arsenm: The pattern will do nothing here, so yes only the definitions
arsenmUnsubmitted
Not Done
ReplyInline Actions

This is also trying to use half mul for integer mul

arsenm: This is also trying to use half mul for integer mul
wdngAuthorUnsubmitted
Not Done
ReplyInline Actions

Could you please say in more details? Thanks!

wdng: Could you please say in more details? Thanks!