This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Add a DAG combine to turn a splat build_vector where the splat elemnt is a bitcast from a vector type into a concat_vector
ClosedPublic

Authored by craig.topper on Jan 15 2018, 5:26 PM.

Details

Summary

For example, a build_vector of i64 bitcasted from v2i32 can be turned into a concat_vectors of the v2i32 vectors with a bitcast to a vXi64 type.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Jan 15 2018, 5:26 PM

Can you upload again with context?

Do you know if we can declare victory over PR34716 after this?
https://bugs.llvm.org/show_bug.cgi?id=34716

Can you upload again with context?

Do you know if we can declare victory over PR34716 after this?
https://bugs.llvm.org/show_bug.cgi?id=34716

We still need to fix the missed load folding on that one.

RKSimon added inline comments.Jan 16 2018, 9:48 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14952 ↗(On Diff #129913)

Also, should this be limited to before LegalOperations in case a backend is trying to do this in reverse?

With context this time.

spatel added inline comments.Jan 17 2018, 7:11 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14950–14951 ↗(On Diff #130094)

Add a formula in the comment to describe the transform?
build_vector [ bitcast (Splat0), bitcast (Splat0), ...] --> bitcast (concat_vectors Splat0)

14953 ↗(On Diff #130094)

Might want to use 'peekThroughBitcast' here...although that reminds me that I still need to figure out if we can remove the loop in there yet.

14952 ↗(On Diff #129913)

That sounds right. I think this is wrong frequently, but nobody notices because it's not exposed in regression tests for in-tree targets. Another option - if we do want to run this after legalization - would be to check if the CONCAT is legal or custom before creating it?

craig.topper added inline comments.Jan 17 2018, 11:33 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14953 ↗(On Diff #130094)

I thought about that, but I would have to still verify that at least one bitcast had existed. My assumption is that if there were multiple bitcasts, they would be combined and cause this user to get re-evaluated right?

spatel added inline comments.Jan 17 2018, 12:49 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14953 ↗(On Diff #130094)

You can't have a build vector of vectors, so I think there has to be a bitcast in order for the isVector() to match.

Multiple bitcasts should get folded, but last time I removed the loop from 'peekThroughBitcasts', I saw regressions. I guess this a consequence of not running to fix-point...you're never quite sure what will get combined. :)

Either way is ok here I think.

Address review comments.

RKSimon accepted this revision.Jan 17 2018, 1:28 PM

LGTM

This revision is now accepted and ready to land.Jan 17 2018, 1:28 PM
This revision was automatically updated to reflect the committed changes.