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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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())) {
----------------
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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68739