This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] Widen illegal width StrictFP vector operations as needed
ClosedPublic

Authored by cameron.mcinally on Jul 25 2018, 9:38 AM.

Details

Summary

Here's a patch to widen illegal StrictFP vector operations.

This code appears to be a real bear at first glance, but you'll notice that the bulk of the code was borrowed from DAGTypeLegalizer::WidenVecRes_BinaryCanTrap(SDNode *N). I modified that code to make the operand handling more generic (unary, binary, and ternary) and to track the Chains attached to StrictFP operation.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.Jul 25 2018, 9:58 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2716 ↗(On Diff #157292)

Is widening with undef safe for a strict instruction?

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2716 ↗(On Diff #157292)

No, it's not safe to insert undef values into a strict instruction, but I don't think that's what this code is doing.

Maybe I'm misunderstanding this code, but I believe this is performing the original operation at a smaller legal width(s) and then concat'ing the result of the operation(s) with undef values to the next largest legal vector type. That should be fine since it won't introduce a trap that did not exist in the user's code.

craig.topper added inline comments.Jul 25 2018, 10:16 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2716 ↗(On Diff #157292)

Ok. I didn't look closely. I just know most of our widening code widens with undef. Maybe the trap code you copied doesn't do that. Let me actually read this patch.

craig.topper added inline comments.Jul 25 2018, 10:39 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2579 ↗(On Diff #157292)

There's an extra space after the equal sign.

2607 ↗(On Diff #157292)

Add an assert here that the operand VT matches the destination VT when we call GetWidenedVector?

2634 ↗(On Diff #157292)

Declare Oper here instead of above.

2663 ↗(On Diff #157292)

Declare Oper here.

2679 ↗(On Diff #157292)

Is the code from here an exact copy/paste from the other function? Can we add a shared helper function?

kpn added a subscriber: kpn.Jul 25 2018, 11:05 AM

Address some of Craig's comments. Will add comments inline for the remaining ones...

cameron.mcinally marked an inline comment as done.

Use a temp for Oper instead of the whole expression.

cameron.mcinally marked 5 inline comments as done.Jul 25 2018, 11:22 AM
cameron.mcinally added inline comments.
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2607 ↗(On Diff #157292)

That assert won't be valid for converts (yet to be added) and possibly other strict instructions.

2679 ↗(On Diff #157292)

It is the same as above (side note: I just noticed that the yellow bar in Phabricator shows where the code was copied from).

This looks like a reasonable thing to do. I'll work on that now...

craig.topper added inline comments.Jul 25 2018, 11:48 AM
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2607 ↗(On Diff #157292)

Agreed, but the GetWidenedVector call isn't valid without that guarantee. You would need to check if the type is also supposed to be widened and if not you would have to do your own widening.

But the variable "VT" and its use in the EXTRACT_SUBVECTOR in the loop below are also wrong if the input type doesn't match the result type. Probably other things as well.

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2607 ↗(On Diff #157292)

Ah, good point. I misunderstood how GetWidenedVector(...) worked.

I'm looking at WidenVecRes_Convert(...) now and suspect that we should also have a separate function to widen the StrictFP converts. Does this sound okay with you?

If so, I'll add the assert to WidenVecRes_StrictFP(...) now and we can revisit the converts once the intrinsics are added and there is a test case.

Refactor common code out of WidenVecRes_BinaryCanTrap(...) and WidenVecRes_StrictFP(...).

Also add an assert before GetWidenedVector(...) is called.

cameron.mcinally marked 2 inline comments as done.

Add a high-level comment to the new CollectOpsToWiden(...) helper function.

cameron.mcinally marked 4 inline comments as done.Jul 27 2018, 8:47 AM

LGTM with that one change.

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2431 ↗(On Diff #157700)

Change "SelectionDAG& DAG" to "SelectionDAG &DAG"

craig.topper accepted this revision.Jul 31 2018, 9:53 AM

Forgot to click accept.

This revision is now accepted and ready to land.Jul 31 2018, 9:53 AM
cameron.mcinally marked an inline comment as done.Aug 1 2018, 6:59 AM
This revision was automatically updated to reflect the committed changes.