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

[volkan]

Summary

No test case as none of the in-tree targets with GlobalISel support has this condition.

[qcolombet]

LGTM with nitpicks

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

574

Period at the end of sentence

577

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.

580

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.

[kristof.beyls]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

580

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.

[qcolombet]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

580

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)

[volkan]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

580

That's what I thought. We can simply return UnableToLegalize for that case.

[qcolombet]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

580

Agreed.

[volkan]

Check if G_FNEG is marked as Lower to avoid ending up with an infinite loop.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

case TargetOpcode::G_FNEG: {
     MIRBuilder.buildFConstant(Zero, ZeroForNegation);
     MIRBuilder.buildInstr(TargetOpcode::G_FSUB)
         .addDef(Res)
         .addUse(Zero)
         .addUse(MI.getOperand(1).getReg());
     MI.eraseFromParent();
     return Legalized;
   }
+  case TargetOpcode::G_FSUB: {
+    // Lower (G_FSUB LHS, RHS) to (G_FADD LHS, (G_FNEG RHS)).
+    // First, check if G_FNEG is marked as Lower. If so, we may
+    // end up with an infinite loop as G_FSUB is used to legalize G_FNEG.
+    if (LI.getAction({G_FNEG, Ty}).first == LegalizerInfo::Lower)
+      return UnableToLegalize;
+    unsigned Res = MI.getOperand(0).getReg();
+    unsigned LHS = MI.getOperand(1).getReg();
+    unsigned RHS = MI.getOperand(2).getReg();
+    unsigned Neg = MRI.createGenericVirtualRegister(Ty);
+    MIRBuilder.buildInstr(TargetOpcode::G_FNEG).addDef(Neg).addUse(RHS);
+    MIRBuilder.buildInstr(TargetOpcode::G_FADD)
+        .addDef(Res)
+        .addUse(LHS)
+        .addUse(Neg);
+    MI.eraseFromParent();
+    return Legalized;
+  }
   }
 }
 
 LegalizerHelper::LegalizeResult
 LegalizerHelper::fewerElementsVector(MachineInstr &MI, unsigned TypeIdx,
                                      LLT NarrowTy) {
   // FIXME: Don't know how to handle secondary types yet.
   if (TypeIdx != 0)