This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Rename $dst operand to $vdst for VOP instructions.
ClosedPublic

Authored by SamWot on Feb 5 2016, 2:09 AM.

Details

Summary

This change renames output operand for VOP instructions from dst to vdst. This is needed to enable decoding named operands for disassembler.

Diff Detail

Repository
rL LLVM

Event Timeline

SamWot updated this revision to Diff 47004.Feb 5 2016, 2:09 AM
SamWot retitled this revision from to [AMDGPU] Rename $dst operand to $vdst for VOP instructions..
SamWot updated this object.
SamWot added subscribers: nhaustov, llvm-commits.
SamWot updated this object.Feb 5 2016, 2:10 AM
arsenm added inline comments.Feb 5 2016, 3:10 AM
lib/Target/AMDGPU/SIInstrInfo.td
1092–1095 ↗(On Diff #47004)

This looks like an unrelated change

1274 ↗(On Diff #47004)

This is a scalar output, so it should not be a vdst

1320 ↗(On Diff #47004)

Is this ever used for instructions with scalar outputs?

1830 ↗(On Diff #47004)

This is a scalar output, so it should not be a vdst

1841 ↗(On Diff #47004)

Ditto

1854 ↗(On Diff #47004)

Ditto

1868 ↗(On Diff #47004)

Ditto

SamWot added inline comments.Feb 5 2016, 7:57 AM
lib/Target/AMDGPU/SIInstrInfo.td
1830 ↗(On Diff #47004)

Here is problem with output operand name. Purpose of this renaming is to match operands names and names of fields in instruction encoding. VOPC instructions with 64-bit encoding use instruction format VOP3e:

class VOP3e <bits<9> op> : Enc64 {
  bits<8> vdst;
  bits<2> src0_modifiers;
  bits<9> src0;
  bits<2> src1_modifiers;
  bits<9> src1;
  bits<2> src2_modifiers;
  bits<9> src2;
  bits<1> clamp;
  bits<2> omod;

  let Inst{7-0} = vdst;
  let Inst{8} = src0_modifiers{1};
  let Inst{9} = src1_modifiers{1};
  let Inst{10} = src2_modifiers{1};
  let Inst{11} = clamp;
  let Inst{25-17} = op;
  let Inst{31-26} = 0x34; //encoding
  let Inst{40-32} = src0;
  let Inst{49-41} = src1;
  let Inst{58-50} = src2;
  let Inst{60-59} = omod;
  let Inst{61} = src0_modifiers{0};
  let Inst{62} = src1_modifiers{0};
  let Inst{63} = src2_modifiers{0};
}

This encoding correspond to VOP3a encoding from spec. It does not have fields for output operands other than vdst. Thats why I renamed operand to $vdst.

Possibly we should change VOPC instructions format to VOP3be as it is stated in spec.

arsenm added inline comments.Feb 5 2016, 10:22 AM
lib/Target/AMDGPU/SIInstrInfo.td
1830 ↗(On Diff #47004)

Yes, this should be fixed. I'm surprised it doesn't matter

SamWot updated this revision to Diff 47447.Feb 10 2016, 5:43 AM

Renamed $vdst to $sdst for VOPC instructions. Added VOP3ce encoding for VOPC.

SamWot added a project: Restricted Project.Feb 12 2016, 6:27 AM

Can you rebase this on top of the latest llvm code?

SamWot updated this revision to Diff 47965.Feb 15 2016, 2:32 AM
SamWot edited edge metadata.
SamWot marked 7 inline comments as done.

Updated diff with latest LLVM master

SamWot updated this revision to Diff 47983.Feb 15 2016, 5:40 AM

Updated DPP encoding to use vdst name instead of dst

arsenm accepted this revision.Feb 15 2016, 11:16 AM
arsenm edited edge metadata.

LGTM with a few nits

lib/Target/AMDGPU/SIInstrInfo.td
1189 ↗(On Diff #47983)

This would be less intrusive if DstVT was the last argument defaulting to i32

1348 ↗(On Diff #47983)

Extra space after =

lib/Target/AMDGPU/VIInstrFormats.td
135 ↗(On Diff #47983)

Extra spaces before =

143 ↗(On Diff #47983)

Ditto

This revision is now accepted and ready to land.Feb 15 2016, 11:16 AM
SamWot updated this revision to Diff 48061.Feb 16 2016, 3:28 AM
SamWot edited edge metadata.
SamWot marked 4 inline comments as done.

Fix issues from Matts review

This revision was automatically updated to reflect the committed changes.