[PATCH] D86393: [GISel] Add combines for unary FP instrs with constant operand

[Michael Kitzan via Phabricator via llvm-commits]

mkitzan added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1443
+  const ConstantFP *MaybeCst = getConstantFPVRegVal(Op, MRI);
+  if (!MaybeCst || !(DstTy == S16 || DstTy == S32 || DstTy == S64))
+    return None;
----------------
arsenm wrote:
> Don't need to bother with the type check?
Probably not anymore


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1459
+  case TargetOpcode::G_FPTRUNC: {
+    if (DstTy == S64) {
+      bool Unused;
----------------
arsenm wrote:
> Why special case S64 and not use getFltSemanticForLLT?
You're right, no longer necessary


================
Comment at: llvm/lib/CodeGen/LowLevelType.cpp:64
+const llvm::fltSemantics &llvm::getFltSemanticForLLT(LLT Ty) {
+  assert(Ty.isScalar() && "Expected a scalar type.");
+  switch (Ty.getSizeInBits()) {
----------------
arsenm wrote:
> I would expect this to just do .getScalarSizeInBits
I figured calling it on the vector element type would likely be the typical intended use, and calling on the aggregate vector type would likely be bug (and wouldn't really make much sense).


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

https://reviews.llvm.org/D86393