[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:311
+  unsigned FPMantissaWidth = IToFP->getType()->getFPMantissaWidth() - 1;
+  FPMantissaWidth = FPMantissaWidth == 63 ? 112 : FPMantissaWidth;
+  // FIXME: As there is no related builtins added in compliler-rt,
----------------
Add comments to indicate that fp80 is extended to fp128?


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:316
+  unsigned FloatWidth = PowerOf2Ceil(FPMantissaWidth);
+  bool isSigned = IToFP->getOpcode() == Instruction::SIToFP;
+
----------------
IsSigned


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:324
+                        Builder.getIntN(BitWidth, FPMantissaWidth + 3));
+  Value *Temp2 = Builder.CreateShl(Builder.getIntN(BitWidth, 1),
+                                   Builder.getIntN(BitWidth, 63));
----------------
Move to line 491 to avoid dead instruction when it is not fp80?


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:326
+                                   Builder.getIntN(BitWidth, 63));
+  Value *Temp3 = Builder.CreateShl(Builder.getIntN(128, 1),
+                                   Builder.getIntN(128, FPMantissaWidth));
----------------
Ditto


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:328
+                                   Builder.getIntN(128, FPMantissaWidth));
+  Value *Temp4 = Builder.CreateSub(Temp3, Builder.getIntN(128, 1));
+
----------------
Ditto


================
Comment at: llvm/lib/CodeGen/ExpandLargeFpConvert.cpp:389
+  Value *Shl =
+      Builder.CreateShl(isSigned ? Sub : IntVal, Builder.getIntN(BitWidth, 1));
+  Builder.CreateBr(SwEpilog);
----------------
It seems there is many coding style like this. Is it possible that create dead code? However I think they will eventually be deleted in ISel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137241