This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Corrected decoding of 16-bit literals
ClosedPublic

Authored by dp on Jul 18 2020, 6:43 AM.

Details

Summary

16-bit literals are encoded as 32-bit values. If high 16-bits of the value is 0xFFFF, the decoded instruction cannot be reassembled.
For example, the following code

0xff,0x04,0x04,0x52,0xcd,0xab,0xff,0xff

is currently decoded as

v_mul_lo_u16_e32 v2, 0xffffabcd, v2

However this literal is actually a 64-bit constant 0x00000000ffffabcd which violates requirements described in the documentation - the truncation is not safe.

This change corrects decoding to make reassembly possible.

Codegen should probably be corrected as well to zero-extend 16 bit values.

Diff Detail

Event Timeline

dp created this revision.Jul 18 2020, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2020, 6:43 AM
arsenm added inline comments.Jul 20 2020, 6:24 AM
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

We currently don't match source modifiers on integer instructions, so there's no situation where a - should appear here

dp marked 2 inline comments as done.Jul 20 2020, 6:58 AM
dp added inline comments.
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

This is not a modifier. '-' is a part of constant. It is handled uniformly for integer and fp operands. See a comment at AMDGPUAsmParser::parseSP3NegModifier:

Currently this modifier is allowed in the following context:

1. Before a register, e.g. "-v0", "-v[...]" or "-[v0,v1]".
2. Before an 'abs' modifier: -abs(...)
3. Before an SP3 'abs' modifier: -|...|

In all other cases "-" is handled as a part
of an expression that follows the sign.

Do you prefer 0xFFFFFFFFFFFFFCB3?

arsenm added inline comments.Jul 20 2020, 7:00 AM
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Yes, but truncated to the 32-bit value, so 0xffffcb3

dp marked 3 inline comments as done.Jul 20 2020, 7:15 AM
dp added inline comments.
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Why do you want it to be truncated to 32 bits? Note that all assembler constants are 64 bit.

Currently we have uniform truncation rules which work for 16-bit and 32-bit literals. They have been working fine for several years. See https://llvm.org/docs/AMDGPUOperandSyntax.html#conversion-of-integer-values

Do you propose to have separate truncation rules for 16 bit values? This does not seem logical and changing the rules may break existing code.

dp marked 2 inline comments as done.Jul 20 2020, 7:18 AM
dp added inline comments.
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Did you mean 'truncated to 16 bits'?

arsenm added inline comments.Jul 20 2020, 7:19 AM
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Because this is encoded as a 32-bit value, so it shouldn't print 16 digits. We don't print signs on hex values anywhere

dp marked an inline comment as done.Jul 20 2020, 7:33 AM
dp added inline comments.
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Assembler currently enables truncation in two cases:

  • when truncated bits are all 0.
  • when truncated bits are all 1 and the value after truncation has its MSB bit set.

Do you suggest to change rules for 16-bit literals to silently truncate low 32 bits w/o any checks?

arsenm added inline comments.Jul 20 2020, 7:58 AM
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

I think we're talking about different things. The asm parser should probably error if you can't truncate to 32-bits without losing bits.

I'm saying the printer should never print something that looks like a 8 byte value by printing more than 8 digits, and should not print a - in front of a hex value

dp marked an inline comment as done.Jul 20 2020, 8:54 AM
dp added inline comments.
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

Currently clBuildProgram generates output which cannot be assembled. Either asm parser or printer needs to be corrected.

I believe the printer should print a 16-bit value, not a 32-bit one. (Otherwise why do we pretend this is a 16-bit operand?) High 16 bits are unused and truncated by parser (except for special cases like packed operands). We are printing high 16 bits only to show what was actually encoded. So when we have non-zero high 16 bits, this is a corner case.

I do not think we should mutilate assembler truncation rules for the sake of handling a special case. I do not like both long hex literals and negative hex literals but this is a lesser evil for my taste.

arsenm added inline comments.Jul 20 2020, 9:17 AM
llvm/test/CodeGen/AMDGPU/add.i16.ll
39

16-bit/4 digits is fine, but having a sign doesn't make sense here

dp updated this revision to Diff 279302.Jul 20 2020, 10:29 AM

Corrected after a discussion with Matt:

  • added 16-bit truncation;
  • printed as unsigned.
dp added a comment.Jul 21 2020, 3:45 PM

Are there any more issues with this change?

arsenm accepted this revision.Jul 21 2020, 3:58 PM
This revision is now accepted and ready to land.Jul 21 2020, 3:58 PM
This revision was automatically updated to reflect the committed changes.