[PATCH] D30671: [GlobalISel] Translate floating-point negation

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:174-185
+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)));
----------------
kristof.beyls wrote:
> volkan wrote:
> > kristof.beyls wrote:
> > > 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
> > 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
> 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?
> 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?


https://reviews.llvm.org/D30671