This is an archive of the discontinued LLVM Phabricator instance.

Fix invalid shufflevector operands
ClosedPublic

Authored by mpflanzer on Jun 30 2015, 7:54 AM.

Details

Summary

This patch fixes bug 23800 ( https://llvm.org/bugs/show_bug.cgi?id=23800#c2 ). There existed a case where the index operand from extractelement was directly used to create a shufflevector mask. Since the index can be of any integral type but the mask must only contain 32 bit integers a 64 bit index operand led to an assertion error later on.

Diff Detail

Event Timeline

mpflanzer updated this revision to Diff 28781.Jun 30 2015, 7:54 AM
mpflanzer retitled this revision from to Convert index operand to 32bit before creating the shuffle mask.
mpflanzer updated this object.
mpflanzer edited the test plan for this revision. (Show Details)
mpflanzer added a subscriber: Unknown Object (MLST).
mpflanzer retitled this revision from Convert index operand to 32bit before creating the shuffle mask to Fix invalid shufflevector operands.Jul 6 2015, 12:29 AM
mpflanzer updated this object.
mpflanzer added a reviewer: rjmccall.
rjmccall added inline comments.Jul 7 2015, 1:24 PM
lib/CodeGen/CGExprScalar.cpp
1224

The comment can just be "shufflemask must use an i32". It'd be nice if you could avoid re-uniquing the constant when it already is an i32.

mpflanzer updated this revision to Diff 29219.Jul 7 2015, 3:53 PM

Avoid re-uniquing the constant when it already is an i32

mpflanzer marked an inline comment as done.Jul 7 2015, 3:56 PM

Is this a proper way to check whether it is already i32?

rjmccall edited edge metadata.Jul 7 2015, 4:49 PM

Yes, that's a fine way to do it.

It would be a little cleaner to pull out a function to do the entire -> i32 conversion, so that you can just have Args.push_back(getAsInt32(CGF, C));

Otherwise LGTM.

mpflanzer updated this revision to Diff 29253.Jul 8 2015, 2:48 AM
mpflanzer edited edge metadata.

Extracted conversion into a helper function

Thanks, LGTM.

mpflanzer accepted this revision.Jul 8 2015, 4:57 PM
mpflanzer added a reviewer: mpflanzer.

Great, thanks for the review!

This revision is now accepted and ready to land.Jul 8 2015, 4:57 PM
This revision now requires review to proceed.Jul 9 2015, 2:57 AM

Sorry for accidentally accepting the revision myself.
I don't have commit rights so someone else will have to push the fix for me.
Thanks again for your help!

Hi Moritz, if you add some test cases then I think will be ready to be committed.

mpflanzer updated this revision to Diff 30679.Jul 27 2015, 12:49 AM

Added test case that fails for the incorrect code generation

Hi Simon, is this test case enough? Clang will fail with an exit code other than 0 if wrong code is generated. The test passes with my changes. Or should I add a check to the test that something like <2 x i64> <i64 0, i64 undef> is not generated?

Hi Simon, is this test case enough? Clang will fail with an exit code other than 0 if wrong code is generated. The test passes with my changes. Or should I add a check to the test that something like <2 x i64> <i64 0, i64 undef> is not generated?

Yes please add a FileCheck stage to the test to give some insight as to what you expect the result to be.

A comment would be useful as well (reference the PR# and a brief explanation of the bug).

mpflanzer updated this revision to Diff 30689.Jul 27 2015, 7:03 AM

Added FileCheck and description to test case

RKSimon accepted this revision.Aug 2 2015, 7:36 AM
RKSimon added a reviewer: RKSimon.

LGTM - I'll commit this for you

This revision is now accepted and ready to land.Aug 2 2015, 7:36 AM
This revision was automatically updated to reflect the committed changes.

Thank you for committing!