Fix invalid shufflevector operands

[mpflanzer]

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.

[rjmccall]

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]

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

[mpflanzer]

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

[rjmccall]

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]

Extracted conversion into a helper function

[rjmccall]

Thanks, LGTM.

[mpflanzer]

Great, thanks for the review!

[mpflanzer]

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!

[RKSimon]

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

[mpflanzer]

Added test case that fails for the incorrect code generation

[mpflanzer]

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?

[RKSimon]

In D10838#212425, @mpflanzer wrote:

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]

Added FileCheck and description to test case

[rjmccall]

Still LGTM.

[RKSimon]

LGTM - I'll commit this for you

[mpflanzer]

Thank you for committing!

lib/CodeGen/CGExprScalar.cpp

if (!VVT) {
       if (isa<ExtVectorElementExpr>(IE)) {
         llvm::ExtractElementInst *EI = cast<llvm::ExtractElementInst>(Init);
 
         if (EI->getVectorOperandType()->getNumElements() == ResElts) {
           llvm::ConstantInt *C = cast<llvm::ConstantInt>(EI->getIndexOperand());
           Value *LHS = nullptr, *RHS = nullptr;
           if (CurIdx == 0) {
             // insert into undef -> shuffle (src, undef)
-            Args.push_back(C);
+            // extractelement index can be any integer type
+            // shufflemask is only allowed to contain 32 bit integers
+            // make sure we can savely convert to 32 bit
+            assert(llvm::ConstantInt::isValueValidForType(CGF.Int32Ty,
+                                                          C->getZExtValue()) &&
+                   "Index operand too large for shufflevector mask!");
+            Args.push_back(Builder.getInt32(C->getZExtValue()));
             Args.resize(ResElts, llvm::UndefValue::get(CGF.Int32Ty));
 
             LHS = EI->getVectorOperand();
             RHS = V;
             VIsUndefShuffle = true;
           } else if (VIsUndefShuffle) {
             // insert into undefshuffle && size match -> shuffle (v, src)
             llvm::ShuffleVectorInst *SVV = cast<llvm::ShuffleVectorInst>(V);