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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM with nitpicks



================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:575
+  case TargetOpcode::G_FSUB: {
+    // Lower (G_FSUB X, Y) to (G_FADD X, (G_FNEG Y))
+    unsigned Res = MI.getOperand(0).getReg();
----------------
Period at the end of sentence


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:578
+    unsigned LHS = MI.getOperand(1).getReg();
+    unsigned RHS = MI.getOperand(2).getReg();
+    unsigned Neg = MRI.createGenericVirtualRegister(Ty);
----------------
I would call the variable X and Y or change the names in the comment to RHS and LHS to have both the code and the comment to match.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:581
+    MIRBuilder.buildInstr(TargetOpcode::G_FNEG).addDef(Neg).addUse(RHS);
+    MIRBuilder.buildInstr(TargetOpcode::G_FADD)
+        .addDef(Res)
----------------
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. 


https://reviews.llvm.org/D30786