This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] RFC: Recognise any_extend_vector_inreg and truncation style shuffle masks
ClosedPublic

Authored by RKSimon on Feb 2 2017, 7:38 AM.

Details

Summary

During legalization we are often creating shuffles (via a build_vector scalarization stage) that are "any_extend_vector_inreg" style masks, and also other masks that are the equivalent of "truncate_vector_inreg" (if we had such a thing).

This RFC is an attempt to match these cases to help undo the effects of just leaving shuffle lowering to handle it - which typically means we lose track of the undefined elements of the shuffles resulting in an unnecessary extension+truncation stage for widened illegal types.

The 2011-10-21-widen-cmp.ll regression can be fixed by making SIGN_EXTEND_VECTOR_IN_REG legal in SSE instead of lowering them to X86ISD::VSEXT (PR31712). An alternative could be to improve target-specific DemandedElts handling at the DAG level (not a small amount of work....).

The regressions encountered in D28537 would be fixed by this.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Feb 2 2017, 7:38 AM

Ping? Any comments?

delena edited edge metadata.Feb 8 2017, 4:24 AM

You can, probably, add more test cases for extension of <8 x i8> to <8 x i16>. You work with illegal types, right?

You can, probably, add more test cases for extension of <8 x i8> to <8 x i16>. You work with illegal types, right?

These are proving very tricky to match in any testable way - the difference being that integer types get promoted while float types get widened. Only widening results in the truncate_vector_inreg(*_extend_vector_inreg()) patterns, It IS possible for promotions to generate something like truncate_vector_inreg(BINOP(*_extend_vector_inreg(), *_extend_vector_inreg())) which could be matched in a later patch?

delena added inline comments.Feb 12 2017, 11:09 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14305

I don't understand why do you need this check.
You check Mask in IsTruncate(), it should be enough.

14313

I'd check at the beginning, before IsTruncate().
if (N00.getValueType().getSizeInBits() != VT.getSizeInBits())

return SDValue();
RKSimon updated this revision to Diff 88723.Feb 16 2017, 6:10 AM

Addressed Elena's comments.

delena added inline comments.Feb 16 2017, 6:27 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14243

This check can be removed.

14245

lower case function name: isAnyExtend

14250

Is !LegalOperations important?

RKSimon added inline comments.Feb 16 2017, 7:00 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14245

Ah - missed that thanks - we're not very consistent when it comes to these.....

14250

For these cases yes - currently ISD::ANY_EXTEND_VECTOR_INREG isn't supported by any targets as a legal/custom operation but should be soon.

RKSimon updated this revision to Diff 88732.Feb 16 2017, 7:37 AM

Removed check - was just there as an 'early out' check but was getting in the way.

delena accepted this revision.Feb 17 2017, 6:33 AM

LGTM, after changing function names to lower case.

This revision is now accepted and ready to land.Feb 17 2017, 6:33 AM
This revision was automatically updated to reflect the committed changes.