This is an archive of the discontinued LLVM Phabricator instance.

[X86] Try to avoid casts around logical vector ops recursively.
ClosedPublic

Authored by fhahn on Jan 10 2020, 10:29 AM.

Details

Summary

Currently PromoteMaskArithemtic only looks at a single operation to
skip casts. This means we miss cases where we combine multiple masks.

This patch updates PromoteMaskArithemtic to try to recursively promote
AND/XOR/AND nodes that terminate in truncates of the right size or
constant vectors.

Diff Detail

Event Timeline

fhahn created this revision.Jan 10 2020, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 10:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

The diff just includes the changes for the newly added tests (which I'll pre-commit before landing this)

Not sure if its reusable but we already have signExtendBitcastSrcVector which we use for the (i16 bitcast (v16i1 x)) combine

llvm/lib/Target/X86/X86ISelLowering.cpp
39815

Very minor, but in most depth search cases we start at Depth = 0 and bail once it reaches at maximum.

39881

Does max depth have to be 8 or would we be better using SelectionDAG::MaxRecursionDepth = 6?

fhahn updated this revision to Diff 237489.Jan 11 2020, 4:33 AM

Use SelectionDAG::MaxRecursionDepth, add example to comment.

fhahn marked 2 inline comments as done.Jan 11 2020, 4:47 AM

Not sure if its reusable but we already have signExtendBitcastSrcVector which we use for the (i16 bitcast (v16i1 x)) combine

It looks like signExtendBitcastSrcVector does something similar, pushing extends down through AND/OR/XOR.

However they seem to be specialized for different cases, signExtendBitcastSrcVector focuses on SETCC defs, while in this patch we only consider truncates where we can use the operand without extending it (and constant vectors which are zero-extended). I think we could generalize and combine them, but I am not sure if either use case would benefit from matching more general patterns. I think that would be best evaluated in a potential follow up, what do you think?

llvm/lib/Target/X86/X86ISelLowering.cpp
39815

Thanks, I've changed the code to be in line with what we do in SelectionDAG, including using SelectionDAG::MaxRecursionLimit, which is conveniently exposed.

39881

8 was arbitrarily chosen, but I agree it makes more sense to re-used SelectionDAG::MaxRecursionDepth

Unit tests: pass. 61771 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

fhahn added a comment.Jan 13 2020, 4:43 PM

clang-tidy: fail. Please fix clang-tidy findings.

clang-tidy is unhappy about the uppercase function names PromoteMaskArithmetic. I'm happy to change them both to lowercase if that's preferred.

RKSimon accepted this revision.Jan 17 2020, 9:09 AM

LGTM - I don't have a problem with keeping PromoteMaskArithmetic tbh

This revision is now accepted and ready to land.Jan 17 2020, 9:09 AM
This revision was automatically updated to reflect the committed changes.