<div dir="ltr">If you have a simple reproduction of a miscompile, feel free to file a PR with it and revert the patch while it's being worked on.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 30, 2013 at 1:58 AM, Evgeniy Stepanov <span dir="ltr"><<a href="mailto:eugeni.stepanov@gmail.com" target="_blank">eugeni.stepanov@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">A simple reproducer:<br>
class S {<br>
public:<br>
char x[1024];<br>
};<br>
<br>
S s[16];<br>
S *p[16];<br>
<br>
void f(void) {<br>
for (unsigned i = 0; i != 16; ++i) {<br>
p[i] = s + i;<br>
}<br>
}<br>
<br>
With -O3:<br>
<br>
%0 = getelementptr inbounds [16 x %class.S]* @s, i64 0, i64 %index<br>
%1 = insertelement <2 x %class.S*> undef, %class.S* %0, i32 0<br>
%2 = insertelement <2 x %class.S*> %1, %class.S* getelementptr ([16<br>
x %class.S]* @s, i64 0, i64 undef), i32 1<br>
<br>
<br>
An unrelated issue: with -O2 this loop is fully unrolled, with -O3 it<br>
is only 2x unrolled. Sounds like a phase order issue?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On Thu, May 30, 2013 at 12:49 PM, Evgeniy Stepanov<br>
<<a href="mailto:eugeni.stepanov@gmail.com">eugeni.stepanov@gmail.com</a>> wrote:<br>
> Hey,<br>
><br>
> with this change the loop in<br>
> PartialDiagnostic::StorageAllocator::StorageAllocator is vectorized<br>
> as:<br>
><br>
> vector.body: ; preds =<br>
> %vector.body, %<a href="http://vector.ph" target="_blank">vector.ph</a><br>
> %index = phi i64 [ 0, %<a href="http://vector.ph" target="_blank">vector.ph</a> ], [ %index.next, %vector.body ]<br>
> %11 = getelementptr inbounds<br>
> %"class.clang::PartialDiagnostic::StorageAllocator"* %this, i64 0, i32<br>
> 0, i64 %index, !dbg !3218<br>
> %12 = insertelement <2 x<br>
> %"struct.clang::PartialDiagnostic::Storage"*> undef,<br>
> %"struct.clang::PartialDiagnostic::Storage"* %11, i32 0<br>
> %13 = getelementptr inbounds<br>
> %"class.clang::PartialDiagnostic::StorageAllocator"* %this, i64 0, i32<br>
> 0, i64 undef, !dbg !3218<br>
> %14 = insertelement <2 x<br>
> %"struct.clang::PartialDiagnostic::Storage"*> %12,<br>
> %"struct.clang::PartialDiagnostic::Storage"* %13, i32 1<br>
> %15 = getelementptr inbounds<br>
> %"class.clang::PartialDiagnostic::StorageAllocator"* %this, i64 0, i32<br>
> 1, i64 %index, !dbg !3218<br>
><br>
><br>
> "undef" in "%13 =" line does not look correct.<br>
><br>
><br>
> On Thu, May 30, 2013 at 8:33 AM, Nick Lewycky <<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>> wrote:<br>
>> Author: nicholas<br>
>> Date: Wed May 29 23:33:38 2013<br>
>> New Revision: 182909<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=182909&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=182909&view=rev</a><br>
>> Log:<br>
>> Swizzle vector inputs if it helps us eliminate shuffles.<br>
>><br>
>> Modified:<br>
>> llvm/trunk/lib/Transforms/InstCombine/InstCombine.h<br>
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp<br>
>> llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombine.h<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombine.h?rev=182909&r1=182908&r2=182909&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombine.h?rev=182909&r1=182908&r2=182909&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombine.h (original)<br>
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombine.h Wed May 29 23:33:38 2013<br>
>> @@ -234,6 +234,7 @@ private:<br>
>> bool WillNotOverflowSignedAdd(Value *LHS, Value *RHS);<br>
>> Value *EmitGEPOffset(User *GEP);<br>
>> Instruction *scalarizePHI(ExtractElementInst &EI, PHINode *PN);<br>
>> + Value *EvaluateInDifferentElementOrder(Value *V, ArrayRef<int> Mask);<br>
>><br>
>> public:<br>
>> // InsertNewInstBefore - insert an instruction New before instruction Old<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp?rev=182909&r1=182908&r2=182909&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp?rev=182909&r1=182908&r2=182909&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp (original)<br>
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp Wed May 29 23:33:38 2013<br>
>> @@ -494,6 +494,241 @@ Instruction *InstCombiner::visitInsertEl<br>
>> return 0;<br>
>> }<br>
>><br>
>> +/// Return true if we can evaluate the specified expression tree if the vector<br>
>> +/// elements were shuffled in a different order.<br>
>> +static bool CanEvaluateShuffled(Value *V, ArrayRef<int> Mask,<br>
>> + unsigned Depth = 100) {<br>
>> + // We can always reorder the elements of a constant.<br>
>> + if (isa<Constant>(V))<br>
>> + return true;<br>
>> +<br>
>> + // We won't reorder vector arguments. No IPO here.<br>
>> + Instruction *I = dyn_cast<Instruction>(V);<br>
>> + if (!I) return false;<br>
>> +<br>
>> + // Two users may expect different orders of the elements. Don't try it.<br>
>> + if (!I->hasOneUse())<br>
>> + return false;<br>
>> +<br>
>> + if (Depth == 0) return false;<br>
>> +<br>
>> + switch (I->getOpcode()) {<br>
>> + case Instruction::Add:<br>
>> + case Instruction::FAdd:<br>
>> + case Instruction::Sub:<br>
>> + case Instruction::FSub:<br>
>> + case Instruction::Mul:<br>
>> + case Instruction::FMul:<br>
>> + case Instruction::UDiv:<br>
>> + case Instruction::SDiv:<br>
>> + case Instruction::FDiv:<br>
>> + case Instruction::URem:<br>
>> + case Instruction::SRem:<br>
>> + case Instruction::FRem:<br>
>> + case Instruction::Shl:<br>
>> + case Instruction::LShr:<br>
>> + case Instruction::AShr:<br>
>> + case Instruction::And:<br>
>> + case Instruction::Or:<br>
>> + case Instruction::Xor:<br>
>> + case Instruction::ICmp:<br>
>> + case Instruction::FCmp:<br>
>> + case Instruction::Trunc:<br>
>> + case Instruction::ZExt:<br>
>> + case Instruction::SExt:<br>
>> + case Instruction::FPToUI:<br>
>> + case Instruction::FPToSI:<br>
>> + case Instruction::UIToFP:<br>
>> + case Instruction::SIToFP:<br>
>> + case Instruction::FPTrunc:<br>
>> + case Instruction::FPExt:<br>
>> + case Instruction::GetElementPtr: {<br>
>> + for (int i = 0, e = I->getNumOperands(); i != e; ++i) {<br>
>> + if (!CanEvaluateShuffled(I->getOperand(i), Mask, Depth-1))<br>
>> + return false;<br>
>> + }<br>
>> + return true;<br>
>> + }<br>
>> + case Instruction::InsertElement: {<br>
>> + ConstantInt *CI = dyn_cast<ConstantInt>(I->getOperand(2));<br>
>> + if (!CI) return false;<br>
>> + int ElementNumber = CI->getLimitedValue();<br>
>> +<br>
>> + // Verify that 'CI' does not occur twice in Mask. A single 'insertelement'<br>
>> + // can't put an element into multiple indices.<br>
>> + bool SeenOnce = false;<br>
>> + for (int i = 0, e = Mask.size(); i != e; ++i) {<br>
>> + if (Mask[i] == ElementNumber) {<br>
>> + if (SeenOnce)<br>
>> + return false;<br>
>> + SeenOnce = true;<br>
>> + }<br>
>> + }<br>
>> + return CanEvaluateShuffled(I->getOperand(0), Mask, Depth-1);<br>
>> + }<br>
>> + }<br>
>> + return false;<br>
>> +}<br>
>> +<br>
>> +/// Rebuild a new instruction just like 'I' but with the new operands given.<br>
>> +/// In the event of type mismatch, the type of the operands is correct.<br>
>> +static Value *BuildNew(Instruction *I, ArrayRef<Value*> NewOps) {<br>
>> + // We don't want to use the IRBuilder here because we want the replacement<br>
>> + // instructions to appear next to 'I', not the builder's insertion point.<br>
>> + switch (I->getOpcode()) {<br>
>> + case Instruction::Add:<br>
>> + case Instruction::FAdd:<br>
>> + case Instruction::Sub:<br>
>> + case Instruction::FSub:<br>
>> + case Instruction::Mul:<br>
>> + case Instruction::FMul:<br>
>> + case Instruction::UDiv:<br>
>> + case Instruction::SDiv:<br>
>> + case Instruction::FDiv:<br>
>> + case Instruction::URem:<br>
>> + case Instruction::SRem:<br>
>> + case Instruction::FRem:<br>
>> + case Instruction::Shl:<br>
>> + case Instruction::LShr:<br>
>> + case Instruction::AShr:<br>
>> + case Instruction::And:<br>
>> + case Instruction::Or:<br>
>> + case Instruction::Xor: {<br>
>> + BinaryOperator *BO = cast<BinaryOperator>(I);<br>
>> + assert(NewOps.size() == 2 && "binary operator with #ops != 2");<br>
>> + BinaryOperator *New =<br>
>> + BinaryOperator::Create(cast<BinaryOperator>(I)->getOpcode(),<br>
>> + NewOps[0], NewOps[1], "", BO);<br>
>> + if (isa<OverflowingBinaryOperator>(BO)) {<br>
>> + New->setHasNoUnsignedWrap(BO->hasNoUnsignedWrap());<br>
>> + New->setHasNoSignedWrap(BO->hasNoSignedWrap());<br>
>> + }<br>
>> + if (isa<PossiblyExactOperator>(BO)) {<br>
>> + New->setIsExact(BO->isExact());<br>
>> + }<br>
>> + return New;<br>
>> + }<br>
>> + case Instruction::ICmp:<br>
>> + assert(NewOps.size() == 2 && "icmp with #ops != 2");<br>
>> + return new ICmpInst(I, cast<ICmpInst>(I)->getPredicate(),<br>
>> + NewOps[0], NewOps[1]);<br>
>> + case Instruction::FCmp:<br>
>> + assert(NewOps.size() == 2 && "fcmp with #ops != 2");<br>
>> + return new FCmpInst(I, cast<FCmpInst>(I)->getPredicate(),<br>
>> + NewOps[0], NewOps[1]);<br>
>> + case Instruction::Trunc:<br>
>> + case Instruction::ZExt:<br>
>> + case Instruction::SExt:<br>
>> + case Instruction::FPToUI:<br>
>> + case Instruction::FPToSI:<br>
>> + case Instruction::UIToFP:<br>
>> + case Instruction::SIToFP:<br>
>> + case Instruction::FPTrunc:<br>
>> + case Instruction::FPExt: {<br>
>> + // It's possible that the mask has a different number of elements from<br>
>> + // the original cast. We recompute the destination type to match the mask.<br>
>> + Type *DestTy =<br>
>> + VectorType::get(I->getType()->getScalarType(),<br>
>> + NewOps[0]->getType()->getVectorNumElements());<br>
>> + assert(NewOps.size() == 1 && "cast with #ops != 1");<br>
>> + return CastInst::Create(cast<CastInst>(I)->getOpcode(), NewOps[0], DestTy,<br>
>> + "", I);<br>
>> + }<br>
>> + case Instruction::GetElementPtr: {<br>
>> + Value *Ptr = NewOps[0];<br>
>> + ArrayRef<Value*> Idx = NewOps.slice(1);<br>
>> + GetElementPtrInst *GEP = GetElementPtrInst::Create(Ptr, Idx, "", I);<br>
>> + GEP->setIsInBounds(cast<GetElementPtrInst>(I)->isInBounds());<br>
>> + return GEP;<br>
>> + }<br>
>> + }<br>
>> + llvm_unreachable("failed to rebuild vector instructions");<br>
>> +}<br>
>> +<br>
>> +Value *<br>
>> +InstCombiner::EvaluateInDifferentElementOrder(Value *V, ArrayRef<int> Mask) {<br>
>> + // Mask.size() does not need to be equal to the number of vector elements.<br>
>> +<br>
>> + assert(V->getType()->isVectorTy() && "can't reorder non-vector elements");<br>
>> + if (isa<UndefValue>(V)) {<br>
>> + return UndefValue::get(VectorType::get(V->getType()->getScalarType(),<br>
>> + Mask.size()));<br>
>> + }<br>
>> + if (isa<ConstantAggregateZero>(V)) {<br>
>> + return ConstantAggregateZero::get(<br>
>> + VectorType::get(V->getType()->getScalarType(),<br>
>> + Mask.size()));<br>
>> + }<br>
>> + if (Constant *C = dyn_cast<Constant>(V)) {<br>
>> + SmallVector<Constant *, 16> MaskValues;<br>
>> + for (int i = 0, e = Mask.size(); i != e; ++i) {<br>
>> + if (Mask[i] == -1)<br>
>> + MaskValues.push_back(UndefValue::get(Builder->getInt32Ty()));<br>
>> + else<br>
>> + MaskValues.push_back(Builder->getInt32(Mask[i]));<br>
>> + }<br>
>> + return ConstantExpr::getShuffleVector(C, UndefValue::get(C->getType()),<br>
>> + ConstantVector::get(MaskValues));<br>
>> + }<br>
>> +<br>
>> + Instruction *I = cast<Instruction>(V);<br>
>> + switch (I->getOpcode()) {<br>
>> + case Instruction::Add:<br>
>> + case Instruction::FAdd:<br>
>> + case Instruction::Sub:<br>
>> + case Instruction::FSub:<br>
>> + case Instruction::Mul:<br>
>> + case Instruction::FMul:<br>
>> + case Instruction::UDiv:<br>
>> + case Instruction::SDiv:<br>
>> + case Instruction::FDiv:<br>
>> + case Instruction::URem:<br>
>> + case Instruction::SRem:<br>
>> + case Instruction::FRem:<br>
>> + case Instruction::Shl:<br>
>> + case Instruction::LShr:<br>
>> + case Instruction::AShr:<br>
>> + case Instruction::And:<br>
>> + case Instruction::Or:<br>
>> + case Instruction::Xor:<br>
>> + case Instruction::ICmp:<br>
>> + case Instruction::FCmp:<br>
>> + case Instruction::Trunc:<br>
>> + case Instruction::ZExt:<br>
>> + case Instruction::SExt:<br>
>> + case Instruction::FPToUI:<br>
>> + case Instruction::FPToSI:<br>
>> + case Instruction::UIToFP:<br>
>> + case Instruction::SIToFP:<br>
>> + case Instruction::FPTrunc:<br>
>> + case Instruction::FPExt:<br>
>> + case Instruction::Select:<br>
>> + case Instruction::GetElementPtr: {<br>
>> + SmallVector<Value*, 8> NewOps;<br>
>> + bool NeedsRebuild = (Mask.size() != I->getType()->getVectorNumElements());<br>
>> + for (int i = 0, e = I->getNumOperands(); i != e; ++i) {<br>
>> + Value *V = EvaluateInDifferentElementOrder(I->getOperand(i), Mask);<br>
>> + NewOps.push_back(V);<br>
>> + NeedsRebuild |= (V != I->getOperand(i));<br>
>> + }<br>
>> + if (NeedsRebuild) {<br>
>> + return BuildNew(I, NewOps);<br>
>> + }<br>
>> + return I;<br>
>> + }<br>
>> + case Instruction::InsertElement: {<br>
>> + uint32_t Element = cast<ConstantInt>(I->getOperand(2))->getLimitedValue();<br>
>> + if (Element >= Mask.size()) {<br>
>> + // Such instructions are valid and exhibit undefined behaviour.<br>
>> + return UndefValue::get(I->getType());<br>
>> + }<br>
>> + Value *V = EvaluateInDifferentElementOrder(I->getOperand(0), Mask);<br>
>> + return InsertElementInst::Create(V, I->getOperand(1),<br>
>> + Builder->getInt32(Mask[Element]), "", I);<br>
>> + }<br>
>> + }<br>
>> + llvm_unreachable("failed to reorder elements of vector instruction!");<br>
>> +}<br>
>><br>
>> Instruction *InstCombiner::visitShuffleVectorInst(ShuffleVectorInst &SVI) {<br>
>> Value *LHS = SVI.getOperand(0);<br>
>> @@ -574,6 +809,16 @@ Instruction *InstCombiner::visitShuffleV<br>
>> if (isRHSID) return ReplaceInstUsesWith(SVI, RHS);<br>
>> }<br>
>><br>
>> + if (isa<UndefValue>(RHS) &&<br>
>> + // This isn't necessary for correctness, but the comment block below<br>
>> + // claims that there are cases where folding two shuffles into one would<br>
>> + // cause worse codegen on some targets.<br>
>> + !isa<ShuffleVectorInst>(LHS) &&<br>
>> + CanEvaluateShuffled(LHS, Mask)) {<br>
>> + Value *V = EvaluateInDifferentElementOrder(LHS, Mask);<br>
>> + return ReplaceInstUsesWith(SVI, V);<br>
>> + }<br>
>> +<br>
>> // If the LHS is a shufflevector itself, see if we can combine it with this<br>
>> // one without producing an unusual shuffle.<br>
>> // Cases that might be simplified:<br>
>><br>
>> Modified: llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll?rev=182909&r1=182908&r2=182909&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll?rev=182909&r1=182908&r2=182909&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll (original)<br>
>> +++ llvm/trunk/test/Transforms/InstCombine/vec_shuffle.ll Wed May 29 23:33:38 2013<br>
>> @@ -153,3 +153,15 @@ define <8 x i8> @test12a(<8 x i8> %tmp6,<br>
>> ret <8 x i8> %tmp3<br>
>> }<br>
>><br>
>> +define <2 x i8> @test13(i8 %x1, i8 %x2) {<br>
>> +; CHECK: @test13<br>
>> +; CHECK-NEXT: insertelement {{.*}} undef, i8 %x1, i32 1<br>
>> +; CHECK-NEXT: insertelement {{.*}} i8 %x2, i32 0<br>
>> +; CHECK-NEXT: add {{.*}} <i8 7, i8 5><br>
>> +; CHECK-NEXT: ret<br>
>> + %A = insertelement <2 x i8> undef, i8 %x1, i32 0<br>
>> + %B = insertelement <2 x i8> %A, i8 %x2, i32 1<br>
>> + %C = add <2 x i8> %B, <i8 5, i8 7><br>
>> + %D = shufflevector <2 x i8> %C, <2 x i8> undef, <2 x i32> <i32 1, i32 0><br>
>> + ret <2 x i8> %D<br>
>> +}<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>