[PATCH] D107187: [amdgpu] Add an enhanced conversion from i64 to f32.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2530
+    // Count the leading zeros.
+    ShAmt = DAG.getNode(ISD::CTLZ_ZERO_UNDEF, SL, MVT::i32, Hi);
+    ShAmt = DAG.getSelect(SL, MVT::i32,
----------------
Why is zero undef OK? The high half could be all zeroes


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:2568
+    // Set the sign bit.
+    assert(Sign);
+    Sign = DAG.getNode(ISD::SHL, SL, MVT::i32,
----------------
This is redundant since it should assert in the getNode


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2095-2096
+    auto HasSameSign = B.buildICmp(CmpInst::ICMP_SGE, S1, X, Zero);
+    auto MaxShAmt = B.buildSelect(S32, HasSameSign, B.buildConstant(S32, 33),
+                                  B.buildConstant(S32, 32));
+    auto LS = B.buildIntrinsic(Intrinsic::amdgcn_sffbh, {S32},
----------------
You need to assign these constants to variables. The evaluation order of function arguments isn't defined so this could result in a different instruction ordering depending on the host compiler


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2118-2119
+  auto Scale = B.buildSub(S32, B.buildConstant(S32, 32), ShAmt);
+  SmallVector<Register, 1> Results;
+  Results.push_back(Dst);
+  B.buildIntrinsic(Intrinsic::amdgcn_ldexp, Results, /*HasSideEffects=*/false)
----------------
You can just do {Dst} directly to the buildInstr call


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107187