Expand constrained FP operations

[cameron.mcinally]

Summary

Add a helper function to expand constrained FP operations as needed.

[cameron.mcinally]

Corrected a small whitespace problem.

[cameron.mcinally]

Updated this patch to handle a constrained vector POWI. The arguments to that function do not have a uniform type.

I have not included a test case for POWI since the intrinsic declaration also needs to be updated to handle vectors. That is a separate issue and probably deserves its own Diff.

[cameron.mcinally]

In D47491#1116472, @cameron.mcinally wrote:

Updated this patch to handle a constrained vector POWI. The arguments to that function do not have a uniform type.

I have not included a test case for POWI since the intrinsic declaration also needs to be updated to handle vectors. That is a separate issue and probably deserves its own Diff.

The LLVM LangRef states that the POWI intrinsic always takes a scalar i32 as the 2nd argument, even when the 1st argument is a vector. That seems wrong, but I'll save the llvm-dev discussion for another time.

I'll have to special case the POWI expansion in light of this...

[cameron.mcinally]

Apologies for the churn. I've left out POWI for now since it's such an oddball. Will handle it in another Diff.

I'll let this Diff sit for review now...

[andrew.w.kaylor]

I'm adding Craig Topper as a reviewer because he knows the vector selection DAG stuff better than I do. The constrained FP handling looks OK to me.

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll

2

I'm guessing you copied these run lines from somewhere else. It doesn't look like you need multiple runs or the extra check prefixes.

6

I think this check needs to do more than this. You should be verifying the complete expansion.

Also, can you add checks for some other instructions. Some of these won't require expansion, right?

[craig.topper]

lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp

1134

This makes an ArrayRef to a temporary std::initializer_list. This is very unsafe.

You just want

EVT ValuesVTs[] = {EltVT, MVT::Other};

1158

Add SDValue in front of this and remove the earlier declaration.

1164

Add SDValue in front of these and remvoe the earlier declaration.

[cameron.mcinally]

Make updates as needed from the code review.

[cameron.mcinally]

Thanks, guys.

I'd also like to point out that this Diff will slightly clobber Ulrich's D45576. But, I suspect that it will be a small change to accommodate the difference.

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll

6

I think this check needs to do more than this. You should be verifying the complete expansion.

Thanks. I've added a check for the upper POW.

Also, can you add checks for some other instructions. Some of these won't require expansion, right?

POW is the only strict operation that I've found that currently needs expansion on X86. That said, I have a full set of tests that cover each strict operation. I could add them if you'd like, but it's a bit superfluous for this change. I do intend to add the full set once other patches are sent upstream. Thought?

[craig.topper]

I'm happy if @andrew.w.kaylor is happy.

[andrew.w.kaylor]

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll

6

I'd really like to see checks of the entire sequence to which this gets expanded. That seems like the only way to be certain it was expanded correctly. In some of the other tests I took a shortcut similar to what you did here because there was a 1-to-1 correspondence between the intrinsic and the instruction to which it was lowered. Even in that case it was probably not an ideal test.

Regarding instructions that aren't supposed to be expanded, what I would like to see here is a test that uses a vector form of the constrained intrinsic and a check that verifies that we generated the corresponding vector instruction.

[craig.topper]

Non-string RINT and NEARBYINT should also be Expand on pre-SSE4.1 targets. So that would be mean we should expand the strict versions too.

[craig.topper]

Err that should have said non-strict not non-string

[cameron.mcinally]

In D47491#1129202, @craig.topper wrote:

Non-string RINT and NEARBYINT should also be Expand on pre-SSE4.1 targets. So that would be mean we should expand the strict versions too.

Ah, you're right. I do not have tests for those locally. I'll add a few.

[cameron.mcinally]

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll

6

Andrew, just to be clear, you'd like to check for more than the Op+MOVH? There's not a lot to the expansion with these. The only thing I left out of the checks is the loads to feed the POW. The rest is epilogue code.

I suspect you missed the change I made to check for the MOVH, but maybe I'm mistaken?

Or maybe you're suggesting a more complicated vector operation? One where we have to generate two scalar operations and then shuffle them back together? I could see some value in that.

%bb.0: # %entry
pushq %rax
.cfi_def_cfa_offset 16
movsd .LCPI0_0(%rip), %xmm0 # xmm0 = mem[0],zero
movsd .LCPI0_1(%rip), %xmm1 # xmm1 = mem[0],zero
callq pow
movlhps %xmm0, %xmm0 # xmm0 = xmm0[0,0]
popq %rax
.cfi_def_cfa_offset 8
retq

[cameron.mcinally]

In D47491#1129607, @cameron.mcinally wrote:

In D47491#1129202, @craig.topper wrote:

Non-string RINT and NEARBYINT should also be Expand on pre-SSE4.1 targets. So that would be mean we should expand the strict versions too.

Ah, you're right. I do not have tests for those locally. I'll add a few.

Looking closer... the math lib calls are also expanded.

[cameron.mcinally]

New patch for conversation...

• Added more vector tests: -> Arith ops remain vector -> Math lib ops are expanded -> Rounding ops are expanded

TODO: Work on the CHECKs to make sure everything is covered.

[craig.topper]

Are you aware of the update_llc_test_checks.py script that will autogenerate the CHECKs for you?

[cameron.mcinally]

Update diff after running update_llc_test_checks.py.

[cameron.mcinally]

In D47491#1129648, @craig.topper wrote:

Are you aware of the update_llc_test_checks.py script that will autogenerate the CHECKs for you?

Oh, nice. Thanks!

I don't get a lot of opportunities to upstream changes, so it's safe to assume my LLVM knowledge is at least 5 years old. ;)

[craig.topper]

It feels like we should be testing non-splat vectors here so we get two calls to the library functions in the output. Rather than one call and a reuse of the result.

[cameron.mcinally]

In D47491#1129736, @craig.topper wrote:

It feels like we should be testing non-splat vectors here so we get two calls to the library functions in the output. Rather than one call and a reuse of the result.

Ok, that seems reasonable...

Would you be okay with somewhat random input to the upper operation? It should be okay functionally, since only the first trap is reported. I ask since it can be tricky to find inputs that can trap for each of the different operations. There are finicky details that are unique to each operation. Seems unnecessarily cumbersome IMO. Thoughts?

[cameron.mcinally]

Update tests to remove splat inputs.

Notice that the input values are not checked, so there's a modicum of false security there. The low/high parts of the result could be incorrectly swapped and the test would not catch it. Other than that, the test is more robust since two operations are generated for expanded operations.

[craig.topper]

LGTM. And I spoke to Andrew at work and he says it looks good to him to.

lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp

class VectorLegalizer {
   SDValue ExpandSELECT(SDValue Op);
   SDValue ExpandLoad(SDValue Op);
   SDValue ExpandStore(SDValue Op);
   SDValue ExpandFNEG(SDValue Op);
   SDValue ExpandFSUB(SDValue Op);
   SDValue ExpandBITREVERSE(SDValue Op);
   SDValue ExpandCTLZ(SDValue Op);
   SDValue ExpandCTTZ_ZERO_UNDEF(SDValue Op);
+  SDValue ExpandStrictFPOp(SDValue Op);
 
   /// Implements vector promotion.
   ///
   /// This is essentially just bitcasting the operands to a different type and
   /// bitcasting the result back to the original type.
   SDValue Promote(SDValue Op);
 
   /// Implements [SU]INT_TO_FP vector promotion.
SDValue VectorLegalizer::LegalizeOp(SDValue Op) {
 
   for (SDNode::value_iterator J = Node->value_begin(), E = Node->value_end();
        J != E;
        ++J)
     HasVectorValue |= J->isVector();
   if (!HasVectorValue)
     return TranslateLegalizeResults(Op, Result);
 
-  EVT QueryType;
+  TargetLowering::LegalizeAction Action = TargetLowering::Legal;
   switch (Op.getOpcode()) {
   default:
     return TranslateLegalizeResults(Op, Result);
+  case ISD::STRICT_FSQRT:
+  case ISD::STRICT_FMA:
+  case ISD::STRICT_FPOW:
+  case ISD::STRICT_FPOWI:
+  case ISD::STRICT_FSIN:
+  case ISD::STRICT_FCOS:
+  case ISD::STRICT_FEXP:
+  case ISD::STRICT_FEXP2:
+  case ISD::STRICT_FLOG:
+  case ISD::STRICT_FLOG10:
+  case ISD::STRICT_FLOG2:
+  case ISD::STRICT_FRINT:
+  case ISD::STRICT_FNEARBYINT:
+    // These pseudo-ops get legalized as if they were their non-strict
+    // equivalent.  For instance, if ISD::FSQRT is legal then ISD::STRICT_FSQRT
+    // is also legal, but if ISD::FSQRT requires expansion then so does
+    // ISD::STRICT_FSQRT.
+    Action = TLI.getStrictFPOperationAction(Node->getOpcode(), 
+                                          Node->getValueType(0));
+    break;
   case ISD::ADD:
   case ISD::SUB:
   case ISD::MUL:
   case ISD::SDIV:
   case ISD::UDIV:
   case ISD::SREM:
   case ISD::UREM:
   case ISD::SDIVREM:
SDValue VectorLegalizer::LegalizeOp(SDValue Op) {
   case ISD::ZERO_EXTEND_VECTOR_INREG:
   case ISD::SMIN:
   case ISD::SMAX:
   case ISD::UMIN:
   case ISD::UMAX:
   case ISD::SMUL_LOHI:
   case ISD::UMUL_LOHI:
   case ISD::FCANONICALIZE:
-    QueryType = Node->getValueType(0);
+    Action = TLI.getOperationAction(Node->getOpcode(), Node->getValueType(0));
     break;
   case ISD::FP_ROUND_INREG:
-    QueryType = cast<VTSDNode>(Node->getOperand(1))->getVT();
+    Action = TLI.getOperationAction(Node->getOpcode(), 
+               cast<VTSDNode>(Node->getOperand(1))->getVT());
     break;
   case ISD::SINT_TO_FP:
   case ISD::UINT_TO_FP:
-    QueryType = Node->getOperand(0).getValueType();
+    Action = TLI.getOperationAction(Node->getOpcode(), 
+                                    Node->getOperand(0).getValueType());
     break;
   case ISD::MSCATTER:
-    QueryType = cast<MaskedScatterSDNode>(Node)->getValue().getValueType();
+    Action = TLI.getOperationAction(Node->getOpcode(),
+               cast<MaskedScatterSDNode>(Node)->getValue().getValueType());
     break;
   case ISD::MSTORE:
-    QueryType = cast<MaskedStoreSDNode>(Node)->getValue().getValueType();
+    Action = TLI.getOperationAction(Node->getOpcode(),
+               cast<MaskedStoreSDNode>(Node)->getValue().getValueType());
     break;
   }
 
   LLVM_DEBUG(dbgs() << "\nLegalizing vector op: "; Node->dump(&DAG));
 
-  switch (TLI.getOperationAction(Node->getOpcode(), QueryType)) {
+  switch (Action) {
   default: llvm_unreachable("This action is not supported yet!");
   case TargetLowering::Promote:
     Result = Promote(Op);
     Changed = true;
     break;
   case TargetLowering::Legal:
     LLVM_DEBUG(dbgs() << "Legal node: nothing to do\n");
     break;
case ISD::SETCC:
     return UnrollVSETCC(Op);
   case ISD::BITREVERSE:
     return ExpandBITREVERSE(Op);
   case ISD::CTLZ:
   case ISD::CTLZ_ZERO_UNDEF:
     return ExpandCTLZ(Op);
   case ISD::CTTZ_ZERO_UNDEF:
     return ExpandCTTZ_ZERO_UNDEF(Op);
+  case ISD::STRICT_FSQRT:
+  case ISD::STRICT_FMA:
+  case ISD::STRICT_FPOW:
+  case ISD::STRICT_FPOWI:
+  case ISD::STRICT_FSIN:
+  case ISD::STRICT_FCOS:
+  case ISD::STRICT_FEXP:
+  case ISD::STRICT_FEXP2:
+  case ISD::STRICT_FLOG:
+  case ISD::STRICT_FLOG10:
+  case ISD::STRICT_FLOG2:
+  case ISD::STRICT_FRINT:
+  case ISD::STRICT_FNEARBYINT:
+    return ExpandStrictFPOp(Op);
   default:
     return DAG.UnrollVectorOp(Op.getNode());
   }
 }
 
 SDValue VectorLegalizer::ExpandSELECT(SDValue Op) {
   // Lower a select instruction where the condition is a scalar and the
   // operands are vectors. Lower this select to VSELECT and implement it
if (TLI.isOperationLegalOrCustom(ISD::CTTZ, Op.getValueType())) {
     SDLoc DL(Op);
     return DAG.getNode(ISD::CTTZ, DL, Op.getValueType(), Op.getOperand(0));
   }
 
   // Otherwise go ahead and unroll.
   return DAG.UnrollVectorOp(Op.getNode());
 }
 
+SDValue VectorLegalizer::ExpandStrictFPOp(SDValue Op) {
+  EVT VT = Op.getValueType();
+  EVT EltVT = VT.getVectorElementType();
+  unsigned NumElems = VT.getVectorNumElements();
+  unsigned NumOpers = Op.getNumOperands();
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  ArrayRef<EVT> ValueVTs = {EltVT, MVT::Other};
+  SDValue Chain = Op.getOperand(0);
+  SDValue Result;
+  SDValue NewChain;
+  SDLoc dl(Op);
+
+  SmallVector<SDValue, 8> OpValues;
+  SmallVector<SDValue, 8> OpChains;
+  for (unsigned i = 0; i < NumElems; ++i) {
+    SDValue ScalarOp;
+    SmallVector<SDValue, 4> Opers;
+    SDValue Idx = DAG.getConstant(i, dl, 
+                                  TLI.getVectorIdxTy(DAG.getDataLayout()));
+
+    // The Chain is the first operand.
+    Opers.push_back(Chain);
+
+    // Now process the remaining operands. 
+    for (unsigned j = 1; j < NumOpers; ++j) {
+      SDValue Oper = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, 
+                                 EltVT, Op.getOperand(j), Idx);
+      Opers.push_back(Oper);
+    }
+ 
+    ScalarOp = DAG.getNode(Op->getOpcode(), dl, ValueVTs, Opers);
+
+    OpValues.push_back(ScalarOp.getValue(0));
+    OpChains.push_back(ScalarOp.getValue(1));
+  }
+
+  Result = DAG.getBuildVector(VT, dl, OpValues);
+  NewChain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, OpChains);
+
+  AddLegalizedOperand(Op.getValue(0), Result);
+  AddLegalizedOperand(Op.getValue(1), NewChain);
+
+  return NewChain;
+}
+
 SDValue VectorLegalizer::UnrollVSETCC(SDValue Op) {
   EVT VT = Op.getValueType();
   unsigned NumElems = VT.getVectorNumElements();
   EVT EltVT = VT.getVectorElementType();
   SDValue LHS = Op.getOperand(0), RHS = Op.getOperand(1), CC = Op.getOperand(2);
   EVT TmpEltVT = LHS.getValueType().getVectorElementType();
   SDLoc dl(Op);
   SmallVector<SDValue, 8> Ops(NumElems);

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll

+; RUN: llc -O3 -mtriple=x86_64-pc-linux < %s | FileCheck --check-prefix=COMMON --check-prefix=NO-FMA --check-prefix=FMACALL64 --check-prefix=FMACALL32 %s
+; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+fma < %s | FileCheck -check-prefix=COMMON --check-prefix=HAS-FMA --check-prefix=FMA64 --check-prefix=FMA32 %s
+
+; CHECK-LABEL: constrained_vector_pow
+; COMMON: pow
+define <2 x double> @constrained_vector_pow() {
+entry:
+  %pow = call <2 x double> @llvm.experimental.constrained.pow.v2f64(
+                                               <2 x double> <double 42.1, double 42.1>,
+                                               <2 x double> <double 3.0, double 3.0>,
+                                               metadata !"round.dynamic",
+                                               metadata !"fpexcept.strict")
+  ret <2 x double> %pow
+}
+
+declare <2 x double> @llvm.experimental.constrained.pow.v2f64(<2 x double>, <2 x double>, metadata, metadata)