[PATCH] D44128: [GISel]: Add helpers for easy building G_FCONSTANT along with matchers

[Volkan Keles via Phabricator via llvm-commits]

volkan added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:291
+  LLT DstTy = MRI->getType(Res);
+  assert(!DstTy.isValid() || (DstTy.getSizeInBits() >= Ty.getSizeInBits()) &&
+                                 "DstReg not large enough for Ty");
----------------
Could you check this in MachineVerifier as well?


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:295
+  auto &Ctx = MF->getFunction().getContext();
+  auto *CFP = ConstantFP::get(Ctx, getAPFloatFromSize(Val, Ty.getSizeInBits()));
+  return buildFConstant(Res, *CFP);
----------------
We already know DstTy here, can't we just use DstTy instead of Ty?


================
Comment at: lib/CodeGen/GlobalISel/Utils.cpp:217
+
+const llvm::fltSemantics &llvm::LLTToAPFloatSemantics(const LLT &Ty) {
+  if (!Ty.isValid())
----------------
We don't need fltSemantics for float and double so I think we can move this to getAPFloatFromSize and always use getAPFloatFromSize to create an APFloat. What do you think?


================
Comment at: lib/CodeGen/GlobalISel/Utils.cpp:240
+  bool Ignored;
+  APFloat APF = APFloat(Val);
+  APF.convert(LLTToAPFloatSemantics(LLT::scalar(Size)),
----------------
This can be simplified as `APFloat APF(Val);` if you prefer.


================
Comment at: unittests/CodeGen/GlobalISel/PatternMatchTest.cpp:292
+  APFloat APF16(.5);
+  APF16.convert(LLTToAPFloatSemantics(s16), APFloat::rmNearestTiesToEven,
+                &Ignored);
----------------
This can be replaced with `getAPFloatFromSize(0.5, 16)`.


https://reviews.llvm.org/D44128