[PATCH] D78494: [AMDGPU][CODEGEN] Added 'A' constraint for inline assembler

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp marked 2 inline comments as done.
dp added a comment.

> I guess the fundamental problem for the 16-bit immediate case is you can't know the use instruction's context. You're really accepting something that's not an inline immediate in the use context, but you have no way of knowing that.

I believe this is a generic problem which affects all supported types because of unknown context.
In the previous version of this change I tried to alleviate this problem by emitting all fp constants as double so they would work in any context.
As you have pointed out, this is unsuitable in some cases where the constant type is importand but for these cases we could have used a 'B' constraint:

  "v_mov_b32 $0, $1", "=v,B"(half 0xH3C00)

However I think the corrent version of this change is simpler and cleaner and it has its logic.
We expect constant type to match assembler operand type, but in the end it is up to programmer to decide how to use the constant in assembly code.

> Actually, could we emit an error in the MC layer if it turns out to not be used in a 16-bit context? That would make more sense, but we don't have a way of tracking the immediate size

Agree, I do not see how this can be done.



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10899
+  } else if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Op)) {
+    Val = C->getValueAPF().bitcastToAPInt().getSExtValue();
+    IsConst = true;
----------------
arsenm wrote:
> This should be getZExtValue? It looks like the tests worked correctly here, but not sure why it didn't matter
This does not really matter because isInlinableLiteralXX truncates input value to XX bits before checking.
However getting sign-extended values is useful to easily recognize negative integer constants and get prettier output.

Consider this code (actually a corner case):
    ... asm "v_mov_b32 $0, $1", "=v,A"(half bitcast (i16 -1 to half))

Currently the code ends up as 
    v_mov_b32 v0, -1

If we replace getSExtValue with getZExtValue, the result would be
    v_mov_b32 v0, 0xFFFF



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

https://reviews.llvm.org/D78494