[GlobalISel] Translate floating-point negation

[kristof.beyls]

lib/CodeGen/GlobalISel/IRTranslator.cpp

174–185

Hi Volkan,

It's unclear to me why we really need a G_FNEG generic opcode.
If the idea is to later on be able to generate a target-specific FNEG instruction, why not have a pattern that can map G_FSUB 0.0, X to a targets specific FNEG, e.g.FNEGDr for AArch64, during the InstructionSelect phase?
Or would the existing DAGISel patterns that contain fneg grow horribly complex if we don't have G_FNEG?

Unless there is a good reason for having an explicit G_FNEG opcode, it seems to me it's best to keep the number of generic opcodes as small as possible?

Thanks,

Kristof

[volkan]

lib/CodeGen/GlobalISel/IRTranslator.cpp

174–185

Hi Kristof,

Sorry for the lack of explanation. We need to have G_FNEG in order to lower G_FSUB in Legalizer as it may not be legal for some targets.

Thanks,
Volkan

[kristof.beyls]

lib/CodeGen/GlobalISel/IRTranslator.cpp

174–185

Thanks, that makes me understand the rationale a lot better - I wasn't aware of targets with this constraint.

I'm still wondering though if in legalization for such a target, it means that G_FSUB should have a custom legalization that somehow doesn't depend on a generic G_FNEG opcode being present? It's not that I'm very against adding G_FNEG when there's a good rationale for it; but I'm wondering if we wouldn't end up with a messy set of generic opcodes if we need to add more and more specialized generic opcodes for all possible out-of-tree targets with non-traditional constraints.
I honestly don't know the best way forward. I'm hoping that @qcolombet may have a better view on this than myself?

Other than that, on this patch, my guess is that this will break existing in-tree targets that already support GlobalISel G_FSUB, like AArch64. So if we go ahead with adding G_FNEG, maybe this patch should also add the necessary support for the in-tree targets that already handle G_FSUB to also handle G_FNEG?

[qcolombet]

Thanks for the confirmation.

Makes sense to me.

[ab]

lib/CodeGen/GlobalISel/IRTranslator.cpp

174–185

Other than that, on this patch, my guess is that this will break existing in-tree targets that already support GlobalISel G_FSUB, like AArch64. So if we go ahead with adding G_FNEG, maybe this patch should also add the necessary support for the in-tree targets that already handle G_FSUB to also handle G_FNEG?

I share Kristof's concern. Doesn't this break AArch64? Should G_FNEG be legalized to G_FSUB?

[qcolombet]

Forgot to point out that you would need to add legalizer support for FNEG => FSUB 0.0, X

include/llvm/CodeGen/GlobalISel/IRTranslator.h

private:
   bool translateAlloca(const User &U, MachineIRBuilder &MIRBuilder);
 
   /// Translate return (ret) instruction.
   /// The target needs to implement CallLowering::lowerReturn for
   /// this to succeed.
   /// \pre \p U is a return instruction.
   bool translateRet(const User &U, MachineIRBuilder &MIRBuilder);
 
+  bool translateFSub(const User &U, MachineIRBuilder &MIRBuilder);
+
   bool translateAdd(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_ADD, U, MIRBuilder);
   }
   bool translateSub(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_SUB, U, MIRBuilder);
   }
   bool translateAnd(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_AND, U, MIRBuilder);
private:
   }
   bool translateAShr(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_ASHR, U, MIRBuilder);
   }
 
   bool translateFAdd(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_FADD, U, MIRBuilder);
   }
-  bool translateFSub(const User &U, MachineIRBuilder &MIRBuilder) {
-    return translateBinaryOp(TargetOpcode::G_FSUB, U, MIRBuilder);
-  }
   bool translateFMul(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_FMUL, U, MIRBuilder);
   }
   bool translateFDiv(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_FDIV, U, MIRBuilder);
   }
   bool translateFRem(const User &U, MachineIRBuilder &MIRBuilder) {
     return translateBinaryOp(TargetOpcode::G_FREM, U, MIRBuilder);

include/llvm/Target/GenericOpcodes.td

def G_SMULH : Instruction {
   let hasSideEffects = 0;
   let isCommutable = 1;
 }
 
 //------------------------------------------------------------------------------
 // Floating Point Unary Ops.
 //------------------------------------------------------------------------------
 
+def G_FNEG : Instruction {
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src);
+  let hasSideEffects = 0;
+}
+
 def G_FPEXT : Instruction {
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins type1:$src);
   let hasSideEffects = 0;
 }
 
 def G_FPTRUNC : Instruction {
   let OutOperandList = (outs type0:$dst);

include/llvm/Target/TargetOpcodes.def

 HANDLE_TARGET_OPCODE(G_FDIV)
 
 /// Generic FP remainder.
 HANDLE_TARGET_OPCODE(G_FREM)
 
 /// Generic FP exponentiation.
 HANDLE_TARGET_OPCODE(G_FPOW)
 
+/// Generic FP negation.
+HANDLE_TARGET_OPCODE(G_FNEG)
+
 /// Generic FP extension.
 HANDLE_TARGET_OPCODE(G_FPEXT)
 
 /// Generic float to signed-int conversion
 HANDLE_TARGET_OPCODE(G_FPTRUNC)
 
 /// Generic float to signed-int conversion
 HANDLE_TARGET_OPCODE(G_FPTOSI)

lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translateBinaryOp(unsigned Opcode, const User &U,
   // Creation of a virtual register needs to have a size.
   unsigned Op0 = getOrCreateVReg(*U.getOperand(0));
   unsigned Op1 = getOrCreateVReg(*U.getOperand(1));
   unsigned Res = getOrCreateVReg(U);
   MIRBuilder.buildInstr(Opcode).addDef(Res).addUse(Op0).addUse(Op1);
   return true;
 }
 
+bool IRTranslator::translateFSub(const User &U, MachineIRBuilder &MIRBuilder) {
+  // -0.0 - X --> G_FNEG
+  if (isa<Constant>(U.getOperand(0)) &&
+      U.getOperand(0) == ConstantFP::getZeroValueForNegation(U.getType())) {
+    MIRBuilder.buildInstr(TargetOpcode::G_FNEG)
+        .addDef(getOrCreateVReg(U))
+        .addUse(getOrCreateVReg(*U.getOperand(1)));
+    return true;
+  }
+  return translateBinaryOp(TargetOpcode::G_FSUB, U, MIRBuilder);
+}
+
 bool IRTranslator::translateCompare(const User &U,
                                     MachineIRBuilder &MIRBuilder) {
   const CmpInst *CI = dyn_cast<CmpInst>(&U);
   unsigned Op0 = getOrCreateVReg(*U.getOperand(0));
   unsigned Op1 = getOrCreateVReg(*U.getOperand(1));
   unsigned Res = getOrCreateVReg(U);
   CmpInst::Predicate Pred =
       CI ? CI->getPredicate() : static_cast<CmpInst::Predicate>(

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

; CHECK: G_STORE [[V2]](s8), [[ADDR]](p0) :: (store singlethread monotonic 1 into %ir.addr)
   %v1 = load atomic i8, i8* %addr acquire, align 1
   store atomic i8 %v1, i8* %addr release, align 1
 
   %v2 = load atomic i8, i8* %addr singlethread seq_cst, align 1
   store atomic i8 %v2, i8* %addr singlethread monotonic, align 1
 
   ret void
 }
+
+define float @test_fneg_f32(float %x) {
+; CHECK-LABEL: name: test_fneg_f32
+; CHECK: [[ARG:%[0-9]+]](s32) = COPY %s0
+; CHECK: [[RES:%[0-9]+]](s32) = G_FNEG [[ARG]]
+; CHECK: %s0 = COPY [[RES]](s32)
+  %neg = fsub float -0.000000e+00, %x
+  ret float %neg
+}
+
+define double @test_fneg_f64(double %x) {
+; CHECK-LABEL: name: test_fneg_f64
+; CHECK: [[ARG:%[0-9]+]](s64) = COPY %d0
+; CHECK: [[RES:%[0-9]+]](s64) = G_FNEG [[ARG]]
+; CHECK: %d0 = COPY [[RES]](s64)
+  %neg = fsub double -0.000000e+00, %x
+  ret double %neg
+}