[PATCH] D146059: [SystemZ] Allow fp/int casting into inline assembly operands

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1115
       weight = CW_Register;
     break;
 
----------------
uweigand wrote:
> Also, this makes me wonder now if we should return `CW_Invalid` here if soft-float (and similarly for 'v' and no-vector).
IIUC, the multiple-alternative decision is the same for all operands, and if one of the operands is invalid in that alternative, the search continues still in other alternatives. So I guess it would make sense to return CW_Invalid and then perhaps end up with a legal alternative, rather than giving the error message in case it would end up with the illegal alternative.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1111
+    else
+      weight = CW_Default;
     break;
----------------
uweigand wrote:
> jonpa wrote:
> > uweigand wrote:
> > > All this `CW_Default` stuff is confusing - isn't this, well, the default?
> > Yeah, I was just following the pattern in use, but I guess it's more readable this way.
> Ah, sorry - I overlooked the default was `CW_Invalid`, not `CW_Default`.   And in fact the default has to be `CW_Invalid`, since this is critical for the various constant constraints like `'I'`.  If the argument is not an in-range constant, this has to be refused.
ouch - yeah, I guess it needs to handle those cases as well.


================
Comment at: llvm/test/CodeGen/SystemZ/inline-asm-fp-int-casting.ll:38
+; Z15-NEXT:    vl %v0, 0(%r3), 3
+; Z15-NEXT:    vrepg %v2, %v0, 1
+; Z15-NEXT:    #APP
----------------
uweigand wrote:
> This difference between z15 and z13 is a bit weird.  Both instruction sequences would be correct and available on both architectures, so I'm not sure why it chooses a different one.  (Also, I'm not actually sure which one performs better ...)   -  That may be something to look at as a follow-on.
The i128 load is bitcasted into f128, and becomes a f128 load. So I guess this then relates to the fact that for z13 we use FP128BitRegClass for f128, and with z14 and later instead the VR128BitRegClass. I guess VectorEnhancements1 is the difference and that makes it worthwhile to use VR128. Yeah, it seems actually better to have two independent instructions and maybe that could be worth trying on z15 as well.


================
Comment at: llvm/test/CodeGen/SystemZ/soft-float-inline-asm-01.ll:10
 
-; CHECK: error: couldn't allocate output register for constraint 'f'
+; CHECK: LLVM ERROR: can't use 'f' constraint with soft-float.
----------------
uweigand wrote:
> Hmm, I think the previous error (reported via `emitInlineAsmError`) is better - this is not an LLVM bug (which is what `report_fatal_error` is primarily for), but rather a user error.
It seems the easy thing to do is to return nullptr for the register class. This diagnostic is not quite as informative, though... (not sure we could do like emitInlineAsmError() and call LLVMContext::emitError() with a better message as the DAG pointer/LLVMContext is not available here).


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

https://reviews.llvm.org/D146059