This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Fix PR33368 (vector extend/truncate optimization)
ClosedPublic

Authored by wolfgangp on Jun 9 2017, 3:58 PM.

Details

Summary

The DAGCombiner, when attempting to turn vector shuffles into vector in_reg_extends and truncates, eliminates extend/truncate sequences when the source and destination element sizes are the same. This fails when the scales of the extensions and truncations involved don't match.

This fixes PR33368.

Diff Detail

Event Timeline

wolfgangp created this revision.Jun 9 2017, 3:58 PM
RKSimon edited edge metadata.Jun 10 2017, 6:12 AM

If possible I'd like to see a regular llc test as well as the mir test.

If possible I'd like to see a regular llc test as well as the mir test.

Actually, I could simply remove the YAML markups and turn this into a regular llc test, if you prefer. The substance of it would not change.

Replaced .mir test with a .ll test, although the test is substantially the same.

Ping...

The test is now a .ll test, do you think this is sufficient? It's relatively tricky to isolate the condition under which this occurs. The original test case was a randomized test.

RKSimon added inline comments.Jun 19 2017, 9:30 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15085

Add a check that (ExtDstSizeInBits % ExtSrcSizeInBits) == 0? Either an assert or early-out

wolfgangp updated this revision to Diff 103118.Jun 19 2017, 3:56 PM

Addressed review comment: added check to ensure that the extension size is a multiple of the source's size.

wolfgangp marked an inline comment as done.Jun 19 2017, 3:57 PM
RKSimon accepted this revision.Jun 26 2017, 1:22 PM

LGTM

This revision is now accepted and ready to land.Jun 26 2017, 1:22 PM
This revision was automatically updated to reflect the committed changes.
zvi added a subscriber: zvi.Jun 28 2017, 2:35 PM

Hi @wolfgangp,

I'm afraid that D34076 adds a combine which replaces the BUILD_VECTOR with a TRUNCATE, so if D34076 will be applied, the test-case in this commit will no longer be covering the bug this commit fixes.
Do you have a suggestion for an alternative test case?

In D34069#794414, @zvi wrote:

Hi @wolfgangp,

I'm afraid that D34076 adds a combine which replaces the BUILD_VECTOR with a TRUNCATE, so if D34076 will be applied, the test-case in this commit will no longer be covering the bug this commit fixes.
Do you have a suggestion for an alternative test case?

Unfortunately not. It may not be possible to create a test case that is sufficiently insulated from future changes to DAGCombine. The only thing that sounds reasonable would be a unit test for the specific routine that had the bug. That would probably be immune against other optimizations, but I'm not even sure that the infrastructure for DAGCombine unit tests exists. In any case, I would be happy with updating the test case to reflect the code that is generated with the new optimization in place, since it will still demonstrate that the right code is generated.

zvi added a comment.Jun 29 2017, 12:47 AM

Err, the patch i am referring to is actually D34077 and not what i mentioned above. I will add you to the review to discuss what can be done about this test.