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

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.

Patch looks OK thanks, just some minor comments inline.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2102-2103
+                  .addUse(Unmerge.getReg(1));
+    auto NotAllSameBits = B.buildICmp(CmpInst::ICMP_NE, S1, LS, AllOnes);
+    auto LS2 = B.buildSelect(S32, NotAllSameBits, LS, MaxShAmt);
+    ShAmt = B.buildSub(S32, LS2, One);
----------------
You could use buildUMin(S32, LS, MaxShAmt) here instead of compare+select.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2105
+    ShAmt = B.buildSub(S32, LS2, One);
+  } else
+    ShAmt = B.buildCTLZ(S32, Unmerge.getReg(1));
----------------
Nit: I think LLVM style is to put braces around the "else" part as well, if there are braces around the "if" part.


================
Comment at: llvm/test/CodeGen/AMDGPU/uint_to_fp.i64.ll:151-152
+; GFX8-NEXT:    s_lshl_b64 s[2:3], s[2:3], s6
+; GFX8-NEXT:    v_cmp_ne_u32_e64 s[4:5], s2, 0
+; GFX8-NEXT:    v_cndmask_b32_e64 v0, 0, 1, s[4:5]
+; GFX8-NEXT:    v_or_b32_e32 v0, s3, v0
----------------
Why does this suddenly start using valu instructions for uniform values?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107187