[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:1199
+      if (useSoftFloat())
+        return std::make_pair(0U, nullptr);
+      if (VT.getSizeInBits() == 64)
----------------
uweigand wrote:
> Isn't this the same the original code (base class) already does?   I don't think we need to change this here at all.
ok


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1463
   }
+  if (ValueVT.getSizeInBits() == 128 && NumParts == 1 && PartVT == MVT::Untyped) {
+    // Inline assmebly operand: f128 -> i128
----------------
uweigand wrote:
> Maybe we should just merge the two if blocks into a something like:
> ```
> if (ValueVT.getSizeInBits() == 128 && NumParts == 1 && PartVT == MVT::Untyped) {
>       if (ValueVT != MVT::i128)
>          Val = DAG.getNode(ISD::BITCAST, SDLoc(Val), MVT::i128, Val);
>       Parts[0] = lowerI128ToGR128(DAG, Val);
> }
> ```
> and similarly below.
Yes - and the getBitcast() has the type check so even better.

I removed the assertions because they didn't seem to be of much use anymore now that we are bitcasting ValueVT and also checking carefully in the if statement. 


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1485
+    SDValue Val_i128 = lowerGR128ToI128(DAG, Parts[0]);
+    return DAG.getNode(ISD::BITCAST, SDLoc(Val_i128), MVT::f128, Val_i128);
+  }
----------------
uweigand wrote:
> It's a bit confusing that this accepts any type with getSizeInBits 128, but then hardcodes `MVT::f128` as result type.   Shouldn't this simply bitcast to `ValueVT` then?
Makes sense.


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

https://reviews.llvm.org/D146059