[PATCH] D137241: [X86] Add ExpandLargeFpConvert Pass and enable for X86

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:75
+///   %sub16 = sub nuw nsw i64 150, %shr
+///   %shr17 = lshr i64 %or, %sub16
+///   %mul = mul nsw i64 %shr17, %conv
----------------
It seems we just truncate the value. Is it because it is rounded to zero?


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:102
+  Value *a1 = nullptr;
+  if (FPMantissaWidth == 10) {
+    if (FPToI->getOpcode() == Instruction::FPToUI) {
----------------
Maybe more readable with `FloatVal->getType()->isHalfTy()`.


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:106
+      a1 = Builder.CreateZExt(a0, IntTy);
+    } else {
+      Value *a0 = Builder.CreateFPToSI(FloatVal, Builder.getIntNTy(32));
----------------
Add comments "FPToSI"?


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:118
+  FPMantissaWidth = FPMantissaWidth == 63 ? 112 : FPMantissaWidth;
+  unsigned FloatWidth = pow(2, int(log2(FPMantissaWidth)) + 1);
+  unsigned ExponentWidth = FloatWidth - FPMantissaWidth - 1;
----------------
PowerOf2Ceil(FPMantissaWidth)?


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:120
+  unsigned ExponentWidth = FloatWidth - FPMantissaWidth - 1;
+  Value *implicitBit = Builder.CreateShl(
+      Builder.getIntN(BitWidth, 1), Builder.getIntN(BitWidth, FPMantissaWidth));
----------------
The first letter should be capital for variable name.


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:122
+      Builder.getIntN(BitWidth, 1), Builder.getIntN(BitWidth, FPMantissaWidth));
+  Value *significandMask =
+      Builder.CreateSub(implicitBit, Builder.getIntN(BitWidth, 1));
----------------
Ditto.


================
Comment at: llvm/test/CodeGen/AArch64/O0-pipeline.ll:19
 ; CHECK-NEXT:       Expand large div/rem
+; CHECK-NEXT:       Expand large fp convert
 ; CHECK-NEXT:       Expand Atomic instructions
----------------
Not sure if it is better to merge convert and div/rem into one pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137241