[PATCH] D70426: [DAGCombiner][RISCV] Avoid FCOPYSIGN folding of legalizing operand casts

[David Sherwood via Phabricator via llvm-commits]

david-arm accepted this revision.
david-arm added a comment.
This revision is now accepted and ready to land.

LGTM. The changes to DAGCombiner seem reasonable to me and I can see an improvement in the X86 code too.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoD.td:234
 def : Pat<(fcopysign FPR64:$rs1, (fneg FPR64:$rs2)), (FSGNJN_D $rs1, $rs2)>;
+def : Pat<(fcopysign FPR64:$rs1, FPR32:$rs2), (FSGNJ_D $rs1, (FCVT_D_S $rs2))>;
+def : Pat<(fcopysign FPR32:$rs1, FPR64:$rs2), (FSGNJ_S $rs1, (FCVT_S_D $rs2, 0b111))>;
----------------
asb wrote:
> luismarques wrote:
> > lenary wrote:
> > > Please can you add a comment to note that these patterns will eventually not be needed, once we improve fcopysign handling?
> > Although that was my original plan, there doesn't seem to be review consensus yet that such changes will be coming and accepted. (If only this patch was committed we would be left at a reasonable place, even if not perfect.)
> I take your point about not wanting to describe future plans where there might not be consensus. I'd put a brief sentence like "These patterns can be removed if..."
nit: If you still plan to go ahead with the patch could you add a brief comment as suggested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70426