[PATCH] D68739: [GISel] Allow ConstantFoldBinOp to consider G_FCONSTANT binary representation for combines

[Marcello Maggioni via Phabricator via llvm-commits]

kariddi marked 2 inline comments as done.
kariddi added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:323-325
+      (&FPVal->getValueAPF().getSemantics() == &APFloat::IEEEdouble() ||
+       &FPVal->getValueAPF().getSemantics() == &APFloat::IEEEsingle() ||
+       &FPVal->getValueAPF().getSemantics() == &APFloat::IEEEhalf())) {
----------------
arsenm wrote:
> kariddi wrote:
> > kariddi wrote:
> > > arsenm wrote:
> > > > Why do you need to whitelist these types?
> > > Are the types that fit a uint64 that we know of, but I guess that could be checked from "the only user" of this function instead
> > Actually, that's not necessarily true ... ConstantFoldBinOp used to use a function that only handled Optional<int64_t> . Now I substituted that with a function that returns APInt (for simplicity), but I wanted to keep the functionality the same as getConstantVRegVal() I guess ...
> > 
> > I guess I can remove that limitation for the Floats, but maintain the limitation fo the integers and then check in ConstantFoldBinOp.
> > 
> > I'm just scared that allowing ConstantFoldBinOp to digest things it never saw before would cause some "unexpected consequence" :-P
> I'm worried about somebody adding bfloat16 or something and then never updating this list. I think it would be fine to just return anything bitcastToAPInt will handle
I tried removing the limitation. It seems not not cause problems , so I'll update the patch


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68739