[PATCH] D30786: [GlobalISel] LegalizerHelper: Lower (G_FSUB X, Y) to (G_FADD X, (G_FNEG Y))

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:581
+    MIRBuilder.buildInstr(TargetOpcode::G_FNEG).addDef(Neg).addUse(RHS);
+    MIRBuilder.buildInstr(TargetOpcode::G_FADD)
+        .addDef(Res)
----------------
kristof.beyls wrote:
> qcolombet wrote:
> > FNEG could be illegal and thus it may be legalized into FSUB.
> > With that in mind, that means we could theoretically end up with an endless loop.
> > How do you think we should address that?
> > Note: I don't expect we solve the problem as part of this commit, but we would need a solution at some point. 
> My feeling is that it's probably better not to try to define generically how G_FSUB should be lowered if the target doesn't support it.
> This patch in the generic code indeed assumes that G_FADD and G_FNEG will be legal when G_FSUB isn't.
> I just think it's better not to make these assumptions in the generic code and just move this to target specific code.
I believe it has value to be in the generic code.
Maybe we should check that FADD and FNEG are legal before applying that transformation and the problem is solved.

FWIW, this is how SDISel does is. (See ~LegalizeDAG.cpp:3236)


https://reviews.llvm.org/D30786