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 ↗

(On Diff #28781)

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!

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

 static llvm::Constant *getMaskElt(llvm::ShuffleVectorInst *SVI, unsigned Idx,
                                   unsigned Off, llvm::Type *I32Ty) {
   int MV = SVI->getMaskValue(Idx);
   if (MV == -1)
     return llvm::UndefValue::get(I32Ty);
   return llvm::ConstantInt::get(I32Ty, Off+MV);
 }
 
+static llvm::Constant *getAsInt32(llvm::ConstantInt *C, llvm::Type *I32Ty) {
+  if (C->getBitWidth() != 32) {
+      assert(llvm::ConstantInt::isValueValidForType(I32Ty,
+                                                    C->getZExtValue()) &&
+             "Index operand too large for shufflevector mask!");
+      return llvm::ConstantInt::get(I32Ty, C->getZExtValue());
+  }
+  return C;
+}
+
 Value *ScalarExprEmitter::VisitInitListExpr(InitListExpr *E) {
   bool Ignore = TestAndClearIgnoreResultAssign();
   (void)Ignore;
   assert (Ignore == false && "init list ignored");
   unsigned NumInitElements = E->getNumInits();
 
   if (E->hadArrayRangeDesignator())
     CGF.ErrorUnsupported(E, "GNU array range designator extension");
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);
+            // shufflemask must use an i32
+            Args.push_back(getAsInt32(C, CGF.Int32Ty));
             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);

cfe/trunk/test/CodeGenOpenCL/vector_shufflevector_valid.cl

+// RUN: %clang_cc1 -emit-llvm -O0 %s -o - | FileCheck %s
+
+// The shuffle vector mask must always be of i32 vector type
+// See http://reviews.llvm.org/D10838 and https://llvm.org/bugs/show_bug.cgi?id=23800#c2
+// for more information about a bug where a 64 bit index operand causes the generation
+// of an invalid mask
+
+typedef unsigned int uint2 __attribute((ext_vector_type(2)));
+
+void vector_shufflevector_valid(void) {
+    //CHECK: {{%.*}} = shufflevector <2 x i32> {{%.*}}, <2 x i32> undef, <2 x i32> <i32 0, i32 undef>
+    (uint2)(((uint2)(0)).s0, 0);
+}