[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:1462
+    if (DstTy == LLT::scalar(16))
+      V.convert(APFloat::IEEEhalf(), APFloat::rmTowardZero, &Unused);
+    else if (DstTy == LLT::scalar(32))
----------------
arsenm wrote:
> Wrong rounding mode
Should the correct rounding mode be `rmNearestTiesToEven`? Is that only for `G_FPTRUNC` to `LLT::scalar(16)` and not `LLT::scalar(32)`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1481
+bool CombinerHelper::matchCombineConstantFoldFpUnary(MachineInstr &MI,
+                                                     double &Cst) {
+  Register DstReg = MI.getOperand(0).getReg();
----------------
arsenm wrote:
> mkitzan wrote:
> > arsenm wrote:
> > > Why go through double instead of preserving the APFloat?
> > Because `GIDefMatchData` wants to have the variable uninitialized, which would call the default ctor of `APFloat` which is private.
> > 
> > See following pseudo code:
> > ```
> > APFLoat MatchDataN; // calls APFloat()
> > if (matchCombineConstantFoldFpUnary(MI, MatchDataN))
> >   replaceInstWithAPFloat(MI, MatchDataN); // dummy function for example
> > ```
> > 
> > The error looks like:
> > ```
> > llvm-project/build/lib/Target/AArch64/AArch64GenPreLegalizeGICombiner.inc:343:11: error: calling a private constructor of class 'llvm::APFloat'
> >   APFloat MatchData23;
> >           ^
> > llvm-project/llvm/include/llvm/ADT/APFloat.h:842:3: note: implicitly declared private here
> >   APFloat() : U(IEEEdouble()) {
> >   ^
> > ```
> I guess you could work around this by keeping it wrapped in Optional<APFloat>
That could work. I ended up liking the current solution with `replaceInstWithFConstant` over my initial prototype where I tried passing around the `APFloat&`, because `buildFConstant(DstOp, double)` will convert the `double` to the appropriate `APFloat` depending on the `LLT` of the `DstOp`. That way we can just take advantage of the existing `replaceInstWithFConstant` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86393