[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)
----------------
volkan wrote:
> qcolombet wrote:
> > 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)
> That's what I thought. We can simply return UnableToLegalize for that case.
Agreed.


https://reviews.llvm.org/D30786