Page MenuHomePhabricator

[AMDGPU][MC][GFX9] Added 16-bit renamed and "_legacy" VALU opcodes
ClosedPublic

Authored by dp on Aug 4 2017, 9:13 AM.

Details

Summary

See Bug 33629: https://bugs.llvm.org//show_bug.cgi?id=33629

Known issues:

  • V_INTERP_P2_F16 and V_INTERP_P2_LEGACY_F16 should be handled in a similar way. This depends on D35902 and will be added separately.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.Aug 4 2017, 9:13 AM
arsenm edited edge metadata.Aug 4 2017, 10:52 AM

I'm confused about what exactly is meant by a legacy instruction. This isn't the first set of instructions renamed to be _legacy, so I think the bit name needs to be more specific.

What exactly is different about mad_legacy for example? Is there a new mad opcode with different behavior? Can we leave it as an MC emission quirk rather than changing codegen to have to be aware of the random instruction rename?

lib/Target/AMDGPU/SIInstrInfo.cpp
701–704 ↗(On Diff #109756)

Move to header

lib/Target/AMDGPU/VOP3Instructions.td
328–329 ↗(On Diff #109756)

The naming convention is the base instruction name is all caps, but the suffix is lowercase

SamWot added inline comments.Aug 5 2017, 12:00 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
702 ↗(On Diff #109756)

You should use subtarget feature rather then check generation directly

lib/Target/AMDGPU/SIInstrInfo.h
186 ↗(On Diff #109756)

It is probably better if you create same functions for other legacy instructions (getFmaF16(), getDivFixupF16()...) although they wouldn't be used.

dp updated this revision to Diff 109882.Aug 5 2017, 9:37 AM
dp retitled this revision from [AMDGPU][MC][GFX9] Added 16-bit renamed and legacy VALU opcodes to [AMDGPU][MC][GFX9] Added 16-bit renamed and "_legacy" VALU opcodes.

Updated and a bit simplified as suggested by Matt and Sam.

SamWot accepted this revision.Aug 7 2017, 8:52 AM

LGTM

This revision is now accepted and ready to land.Aug 7 2017, 8:52 AM
This revision was automatically updated to reflect the committed changes.