Page MenuHomePhabricator

Vector constrained FP intrinsics
AbandonedPublic

Authored by cameron.mcinally on May 16 2018, 11:27 AM.

Details

Summary

Let me preface this by noting that I'm not yet comfortable with the constrained FP intrinsics implementation, so I'm posting this patch to initiate a conversation on how I should proceed...

I wrote some tests to exercise the vector variants of the constrained FP intrinsics and found that some of the vector lib calls may need special handling. E.g. vector POW on X86 is expanded. This will require special handling of STRICT_FPOW in VectorLegalizer::LegalizeOp(...).

To handle STRICT_FPOW, I followed the existing code in SelectionDAGLegalize::LegalizeOp(...), since I believe this is in line with the intended implementation for the STRICT_XXX opcodes.

That being said, it may be more maintainable to reuse a helper function that converts a STRICT_XXX into its corresponding XXX node early in the LegailizeOp(...) functions. For example,

if (Node->isStrictFPOpcode())
  Node = DAG.mutateStrictFPToFP(Node);

Thoughts?

Diff Detail

Event Timeline

cameron.mcinally edited the summary of this revision. (Show Details)May 16 2018, 11:51 AM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
312

Do you actually want to mutate the node at this point?

There's a function, getStrictFPOpcodeAction() that does something very close to what you are doing here except without mutating the node. Would moving that to an accessible location do what you need?

lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
312

Ah, I see the problem now. The splitting and expanding code needs to be taught how to handle the chain on constrained operations. I was inadvertently trying to mutate away the chain. This will probably take a while to sort out...

Thanks, Andrew.

In addition to what Andrew said, there's another reason not to mutate this early: sooner or later, we'll need to give the target the chance to actually model strict operations differently (e.g. via more precise tracking of the FP status bit dependencies). At this point, the information which operations were strict must be preserved until the MI level anyway.

See e.g. my proposed patch here:
https://reviews.llvm.org/D45576

...

See e.g. my proposed patch here:
https://reviews.llvm.org/D45576

Oh, interesting. So the second comment on D45576 suggests that there's more design for this than I've seen so far. I'll close this Diff, if there are not objections. It's pretty clear that this is not the correct path going forward.

Ulrich, is there support I can provide you without stepping on your toes? Maybe I could write tests to exercise the intrinsics? Or something else?

cameron.mcinally abandoned this revision.May 22 2018, 12:39 PM

Oh, interesting. So the second comment on D45576 suggests that there's more design for this than I've seen so far. I'll close this Diff, if there are not objections. It's pretty clear that this is not the correct path going forward.

On the other hand, the problem you've noticed here is of course still real, and needs to be addressed one way or the other. It looks like we will have to have support for properly legalizing STRICT_ operations on unsupported (vector) types. This isn't really addressed by my patch either. (I didn't even realize there was a problem; I had not looked a STRICT operations on vector types at all so far.)

Ulrich, is there support I can provide you without stepping on your toes? Maybe I could write tests to exercise the intrinsics? Or something else?

If you still want to go ahead and implement legalization along the lines of your previous comment ("The splitting and expanding code needs to be taught how to handle the chain on constrained operations."), that wouldn't be stepping on my toes at all :-)

Hi Ulrich,

I have an updated patch ready to go to support expanding strict operations.

I also have a supporting patch waiting for review, namely D47380. I'm assuming that a standalone prerequisite patch is preferred to a blob of code all at once. Please correct me if I'm wrong...