Page MenuHomePhabricator

[AMDGPU][MC][GFX8][GFX9] Corrected names of integer v_{add/addc/sub/subrev/subb/subbrev}

Authored by dp on Nov 15 2017, 9:32 AM.



Summary of changes in gfx8:

  • gfx7's v_add_i32, v_sub_i32, v_subrev_i32 are renamed to v_*_u32.

Summary of changes in gfx9:

  • gfx7's v_add_i32, v_sub_i32, v_subrev_i32, v_addc_u32, v_subb_u32, v_subbrev_u32 are renamed to v_*co_u32.
  • added gfx9-specific v_add_u32, v_sub_u32, v_subrev_u32, v_add_i32, v_sub_i32.

See bug 34765:

Note that changes in script-generated tests (gfx*_asm_all.s, etc) are very large. They are not included in the diff because of Differential limitations.

Diff Detail


Event Timeline

dp created this revision.Nov 15 2017, 9:32 AM
arsenm added inline comments.Nov 15 2017, 12:04 PM
100 ↗(On Diff #123036)

Why are you getting rid of this? We need to do something to understand the register def changes (although I don't think I ever committed the patch that uses this). We probably need to introduce new opcodes for the gfx9 variants that preserve the high bits that have this set (which may or may not need this bit)

84 ↗(On Diff #123036)

Since this is a GFX9 specific check, this shouldn't need the regex on the add name.

I have a patch that starts selecting the carry-outless versions that I'll probably post today. The test changes are a subset of these, so it might be slightly easier to commit my patch first.

dp added inline comments.Nov 16 2017, 3:03 AM
100 ↗(On Diff #123036)

Actually, F16_ZFILL was introduced to allow mapping of pseudo to MC via a separate table. Currently this bit has no other uses.

I renamed it to indicate its purpose clearly because other GFX9 opcodes also requires special mapping.

Should I preserve F16_ZFILL and add a separate bit renamedInGFX9?

84 ↗(On Diff #123036)

Sure, I'll correct this. Please let me know when you submit your changes.

rampitec added inline comments.Nov 16 2017, 3:41 PM
717 ↗(On Diff #123036)

This is not needed anymore.

741 ↗(On Diff #123036)

This is not needed anymore.

dp marked 6 inline comments as done.Nov 20 2017, 7:10 AM
dp updated this revision to Diff 123590.Nov 20 2017, 7:19 AM

Updated and corrected as suggested by Matt and Stas

This revision is now accepted and ready to land.Nov 20 2017, 9:16 AM
This revision was automatically updated to reflect the committed changes.