[PATCH] D84098: [AMDGPU][MC] Corrected decoding of 16-bit literals

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp marked an inline comment as done.
dp added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/add.i16.ll:39
 ; VI: flat_load_ushort [[A:v[0-9]+]]
-; VI: v_add_u16_e32 [[ADD:v[0-9]+]], 0xfffffcb3, [[A]]
+; VI: v_add_u16_e32 [[ADD:v[0-9]+]], -0x34d, [[A]]
 ; VI-NEXT: buffer_store_short [[ADD]]
----------------
arsenm wrote:
> dp wrote:
> > arsenm wrote:
> > > dp wrote:
> > > > dp wrote:
> > > > > arsenm wrote:
> > > > > > dp wrote:
> > > > > > > arsenm wrote:
> > > > > > > > We currently don't match source modifiers on integer instructions, so there's no situation where a - should appear here
> > > > > > > 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?
> > > > > > Yes, but truncated to the 32-bit value, so 0xffffcb3
> > > > > 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.
> > > > Did you mean 'truncated to 16 bits'?
> > > 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
> > 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?
> 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84098/new/

https://reviews.llvm.org/D84098