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

Volkan Keles via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 10 09:45:53 PST 2017


volkan 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)
----------------
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.


https://reviews.llvm.org/D30786





More information about the llvm-commits mailing list