This is an archive of the discontinued LLVM Phabricator instance.

Expand constrained FP POWI
ClosedPublic

Authored by cameron.mcinally on Jun 13 2018, 1:29 PM.

Details

Summary

This is a continuation of D47491...

POWI is an oddball math intrinsic since its 2nd argument is always a scalar integer. Add a helper function to expand STRICT_FPOWI.

Also, update the SmallVector value for both ExpandStrictFPPOWI(...) and ExpandStrictFPOp(...). The new value, 32, was chosen assuming 1024b vectors with 32b FP being the smallest element size. Special thanks to Andy for decoding how SmallVector works.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.Jun 13 2018, 1:44 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1154 ↗(On Diff #151237)

Why couldn't we just detect that the type of Op.getOperand(j) is a scalar type and skip the extract? Then you could handle POWI in in this code without the separate function. Maybe with some asserts to ensure that type is vector everything but the last operand of POWI.

1192 ↗(On Diff #151237)

There's no reason for Opers to be a SmallVector.

You could just do

SDValue Opers[] = { Chain, Base, Power };
test/CodeGen/X86/vector-constrained-fp-intrinsics.ll
337 ↗(On Diff #151237)

FMA is missing? I didn't catch it in the other patch.

342 ↗(On Diff #151237)

sqrt.v4f64 doesn't appear to be used.

craig.topper added inline comments.Jun 13 2018, 1:48 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
729 ↗(On Diff #151237)

Should STRICT_FMUL/DIV/ADD/SUB be here even though I don't know how to test them? If some target did it someday, we would end up the default case which would do the wrong thing.

lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
729 ↗(On Diff #151237)

Ah, you're right. That's a typo.

There could definitely be a RISC-like target without vector arith ops.

1154 ↗(On Diff #151237)

Good call. I was hung up on checking the FPOWI opcode and missed the obvious...

test/CodeGen/X86/vector-constrained-fp-intrinsics.ll
337 ↗(On Diff #151237)

I'll add those...

342 ↗(On Diff #151237)

It's debugging junk. I'll remove it.

lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
729 ↗(On Diff #151237)

These are also missing from SelectionDAGLegalize::LegalizeOp(...). I'll roll that change into this one, if there are no objections.

Suffering a little bit of project creep fixing up bits of code I pass on the way, but I think I've incorporated all of Craig's suggestions. Thanks again, Craig.

Also added two FMA tests and pulled the RUN lines from test/CodeGen/X86/fp-intrinsics.ll. I hope that is okay...

cameron.mcinally marked 10 inline comments as done.Jun 13 2018, 3:41 PM
cameron.mcinally added inline comments.
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1154 ↗(On Diff #151237)

I made this change, but purposefully did not add an ASSERT checking for POWI. I suspect the fix I made is general enough. And, we might possibly see similar arguments when we get to the CMP/CVT/etc intrinsics. Okay with you, Craig?

1192 ↗(On Diff #151237)

This whole function was removed.

cameron.mcinally marked 4 inline comments as done.Jun 13 2018, 3:44 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1154 ↗(On Diff #151237)

Should Op.getOperand(j) be snapped into a temp since there is more than one use? I'm not sure of the LLVM coding standards on that.

craig.topper added inline comments.Jun 13 2018, 4:42 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1154 ↗(On Diff #151237)

Yeah start with

SDValue Oper = Op.getOperand(j)

Then if its a vector just

Oper = DAG.getNodE(ISD::EXTRACT_VECTOR_ELT, dl, EltVT, Oper, Idx)

Then you don't need the else at all.

Create a temp for Op.getOperand(j).

cameron.mcinally marked an inline comment as done.Jun 13 2018, 8:27 PM
This revision is now accepted and ready to land.Jun 15 2018, 10:52 AM
This revision was automatically updated to reflect the committed changes.