Page MenuHomePhabricator

AMDGPU: Rename add/sub with carry out instructions
ClosedPublic

Authored by arsenm on Jul 14 2020, 3:36 PM.

Details

Summary

The hardware has created a real mess in the naming for add/sub, which
have been renamed basically every generation. Switch the carry out
pseudos to have the gfx9/gfx10 names. We were using the original SI/CI
v_add_i32/v_sub_i32 names. Later targets reintroduced these names as
carryless instructions with a saturating clamp bit, which we do not
define. Do this rename so we can unambiguously add these missing
instructions.

The carry-in versions should also be renamed, but at least those had a
consistent _u32 name to begin with. The 16-bit instructions were also
renamed, but aren't ambiguous.

This does regress assembler error message quality in some cases. In
mismatched wave32/wave64 situations, this will switch from
"unsupported instruction" to "invalid operand", with the error
pointing at the wrong position. I couldn't quite follow how the
assembler selects these, but the previous behavior seemed accidental
to me. It looked like there was a partial attempt to handle this which
was never completed (i.e. there is an AMDGPUOperand::isBoolReg but it
isn't used for anything).

Diff Detail

Event Timeline

arsenm created this revision.Jul 14 2020, 3:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 3:36 PM
foad added a comment.Jul 15 2020, 2:02 AM

Sounds great to me, but I'm not much of an expert on instruction definitions.

llvm/lib/Target/AMDGPU/VOP2Instructions.td
1275

"were"

I have no objections but I'll ask Dmirty to check this from asm/dasm side.

No objections, but it would nice to hear from @dp wrt asm error messages.

dp added a comment.Jul 15 2020, 12:32 PM

Sure, I'll take a look tomorrow.

dp accepted this revision.Jul 16 2020, 8:33 AM

LGTM.

Inconsistent error messages will be addressed separately - this is a known issue.

This revision is now accepted and ready to land.Jul 16 2020, 8:33 AM