[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

575

Period at the end of sentence

578

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.

581

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

581

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

581

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

581

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

[qcolombet]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

581

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 X, Y) to (G_FADD X, (G_FNEG Y))
+    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)