[PATCH] D105525: [GISel] Add fpext/fptrunc combines

[Patrick Nappa via Phabricator via llvm-commits]

pnappa added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:346
 
 // Fold fp_op(cst) to the constant result of the floating point operation.
 def constant_fp_op_matchinfo: GIDefMatchData<"Optional<APFloat>">;
----------------
foad wrote:
> I didn't know we had these as combines. Do we really want this, instead of constant-folding them in CSEMIRBuilder? See D99036 and D104528 for some steps in that direction.
I don't know the answer to this, any one else want to chime in?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2490
+  // floating point types, but will have altered precision when folded to
+  // float. We will only allow this fold for Unsafe and 8bit cases.
+  Register SrcInputReg = SrcMI->getOperand(1).getReg();
----------------
paquette wrote:
> foad wrote:
> > Is there a precedent for using "UnsafeFPMath" to enable this particular unsafe combine?
> This looks really similar to the `isKnownExactCastIntToFP` case in `InstCombinerImpl::visitFPExt`.
> 
> Taking a peek in `isKnownExactCastIntToFP` doesn't have any mention of unsafe math. I couldn't find anything in SelectionDAG either (but maybe I missed something?)
> 
> (The checks in `isKnownExactCastIntToFP` seem to do a lot of work actually; I wonder if it would make sense to do something similar here? I guess that could be a follow-up patch.)
Precedent, as in have we done something similar before, enabling a different combine depending on fastmath? Or as in, does this optimisation help in practice? 

Yes to both, I think, but the latter is fairly niche


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

https://reviews.llvm.org/D105525