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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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]]
----------------
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


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

https://reviews.llvm.org/D84098