[PATCH] D23313: X86: FMA intrinsic + FNEG - sequence optimization

[Sanjay Patel via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D23313#511163, @delena wrote:

> In https://reviews.llvm.org/D23313#511156, @RKSimon wrote:
>
> > Instead of adding the forceLegalization approach why can't we just expand combineFMA to support the negation of inputs for all X86ISD FMA/AVX512 opcodes? A helper function might be necessary (to detect FNEG or FXOR signbit-flipping) but that should be all.
>
>
> When we receive X86ISD FMA/AVX512 opcodes, FNEG does not exist any more. It is lowered to FXOR and this is the problem. Analysis of FXOR requires visiting load and visiting constant pool and checking the value of the integer constant under the FP load.
>  That's why I want to delay FNEG legalization.


The premature lowering to load from constant pool is the real problem isn't it? If we didn't do that, then you'd just match the expected X86ISD::FXOR with -1 pattern.

There's a FIXME in LowerFABSorFNEG() about making this better with optsize. I think we should fix this now rather than adding a work-around for something that's broken.

I didn't check if the failures due to this change are just due to load folding diffs or other problems, but the basic patch is just delete the bogus code in LowerFABSorFNEG():

Index: lib/Target/X86/X86ISelLowering.cpp
=========================================

- lib/Target/X86/X86ISelLowering.cpp	(revision 278598)

+++ lib/Target/X86/X86ISelLowering.cpp	(working copy)
@@ -14519,7 +14519,7 @@

  // decide if we should generate a 16-byte constant mask when we only need 4 or
  // 8 bytes for the scalar case.
   

- MVT LogicVT;

+  EVT LogicVT;

  MVT EltVT;
  unsigned NumElts;
   

@@ -14543,18 +14543,10 @@

  }
   
  unsigned EltBits = EltVT.getSizeInBits();

- LLVMContext *Context = DAG.getContext(); // For FABS, mask is 0x7f...; for FNEG, mask is 0x80... APInt MaskElt = IsFABS ? APInt::getSignedMaxValue(EltBits) : APInt::getSignBit(EltBits);
- Constant *C = ConstantInt::get(*Context, MaskElt);
- C = ConstantVector::getSplat(NumElts, C);
- const TargetLowering &TLI = DAG.getTargetLoweringInfo();
- SDValue CPIdx = DAG.getConstantPool(C, TLI.getPointerTy(DAG.getDataLayout()));
- unsigned Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlignment();
- SDValue Mask = DAG.getLoad(
- LogicVT, dl, DAG.getEntryNode(), CPIdx,
- MachinePointerInfo::getConstantPool(DAG.getMachineFunction()), Alignment);

+  SDValue Mask = DAG.getConstant(MaskElt, dl, LogicVT.changeVectorElementTypeToInteger());


Repository:
  rL LLVM

https://reviews.llvm.org/D23313