Fix IRBuilder CreateBitOrPointerCast for vector types

[sbaranga]

Summary

This function was not taking into account that the
input type could be a vector, and wasn't properly
working for vector types.

This caused an assert when building spec2k6 perlbmk for armv8.

[jmolloy]

This LGTM, but please wait for someone else to check it over.

Also, testcase?

[eugenis]

Verified, this patch fixes the crash in http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/20070/steps/build%20llvm-symbolizer%20android%2Farm/logs/stdio

[rengolin]

LGTM, too.

A test case might be hard to produce, but I can give you four candidates:

• Your original spec, anonymized
• Clang (see our self-hosting bots)
• Android (see Eugenis bot)
• Possibly PAQ8p on AArch64 (see our self-hosting bots)

Because this patch is the solution to broken bots, I accept it going as is, without test, for now.

Because the original patch did not break our bots straight away (on self-hosting clang), I think it triggers more Clang-alignment problems, but that's for a different investigation.

Please commit. You can work on the test on a subsequent commit.

cheers,
--reanato

[mzolotukhin]

Hi Silviu,

What was the assertion and why didn't we get it when you tested specs before? Is the data that you posted before on the performance/code size/compile time still valid?

Thanks,
Michael

include/llvm/IR/IRBuilder.h

1355

This doesn't look right. What if we have vectors of different sizes (i.e. V is <8 x i32> and DestTy is <4 x i32>)?

[eugenis]

Almost minimized test case, build with --target=armv7-linux-gnueabi -std=c++11 -O2 -c

class A {

char *Data;
int Length;

public:

A() : Data(), Length() {}

};
void *operator new(unsigned, void *);
class B {
public:

A *resize_I;
void m_fn1() {
  for (auto E = m_fn2(); resize_I != E; ++resize_I)
    new (resize_I) A;
}
A *m_fn2();

};

class C {

void m_fn3(B &) const;

};
void C::m_fn3(B &p1) const const { p1.m_fn1(); }

[llvm-commits]

Thanks, Evgeniy! Here is IR testcase:

• msg-20805-303.txt162 BDownload
• reduced.ll2 KBDownload
• msg-20805-302.txt3 KBDownload

[silviu.baranga]

Hi,

Thanks for the reduced testcase!

The assertion was caused by a call to this function (CreateBitOrPointerCast) from a <4 x struct.. *> to <4 x i32> (which is similar with this case). I don't think it should assert in this case.

I didn't see these problems in my testing. The benchmark was compiling fine and producing valid results. The fact that we are seeing these issues is strange. Could it be caused by some parallel commit?

The testing results should still be valid.

I don't have access to my testing setup at the moment. I'll try to fix this as soon as I can (in about 9 hours?).

Thanks,
Silviu

include/llvm/IR/IRBuilder.h

1355

Would calling this function to create a cast from <8 x i32> to <4 xi32> be valid? Perhaps we should add an assert here.

[mzolotukhin]

include/llvm/IR/IRBuilder.h

1355

I think we don't need to change this condition, as it only introduces unneeded ambiguity (like in the case above). And, actually, we don't get anything from it, right?

1357–1362

These changes look fine (and we don't need to introduce asserts since we have required checks in CreateCast... routines).

[sbaranga]

Addressed review comments and added reduced testcase.

[sbaranga]

Also add some comments to the test case.

[sbaranga]

Hi,

I've committed this in r246759 to unbreak the bots.
Michael, are you ok with the committed version?

Thanks,
Silviu

[sbaranga]

include/llvm/IR/IRBuilder.h

1355

Yes, I think you are right. I've removed that change from the final diff.

[sbaranga]

I've also investigated how we missed this issue: there is no spec2k6 data (just for aarch64) on the ARM review. For ARM we just had lnt/spec2k data. I think the most recent run that I had for ARM with spec2k6 and interleaved accesses enabled was from about 2 months ago (which is why I was sure that I did run this on ARM).

Thanks,
Silviu

[mzolotukhin]

Yes, thanks.
I imagine the testcase might be reduced further though.

[sbaranga]

Further reduced the testcase in r246848.

Thanks,
Silviu

include/llvm/IR/IRBuilder.h

if (V->getType() == DestTy)
       return V;
     if (Constant *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreateIntCast(VC, DestTy, isSigned), Name);
     return Insert(CastInst::CreateIntegerCast(V, DestTy, isSigned), Name);
   }
 
   Value *CreateBitOrPointerCast(Value *V, Type *DestTy,
                                 const Twine &Name = "") {
     if (V->getType() == DestTy)
       return V;
-    if (V->getType()->isPointerTy() && DestTy->isIntegerTy())
+    if (V->getType()->getScalarType()->isPointerTy() &&
+        DestTy->getScalarType()->isIntegerTy())
       return CreatePtrToInt(V, DestTy, Name);
-    if (V->getType()->isIntegerTy() && DestTy->isPointerTy())
+    if (V->getType()->getScalarType()->isIntegerTy() &&
+        DestTy->getScalarType()->isPointerTy())
       return CreateIntToPtr(V, DestTy, Name);
 
     return CreateBitCast(V, DestTy, Name);
   }
 private:
   // \brief Provided to resolve 'CreateIntCast(Ptr, Ptr, "...")', giving a
   // compile time error, instead of converting the string to bool for the
   // isSigned parameter.
   Value *CreateIntCast(Value *, Type *, const char *) = delete;

test/Transforms/LoopVectorize/ARM/vector_cast.ll

+; RUN: opt -loop-vectorize -tbaa -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv7--linux-gnueabi"
+
+%class.C = type { i8 }
+%class.B = type { %class.A* }
+%class.A = type { i8*, i32 }
+
+; CHECK-LABEL: test0
+define void @test0(%class.C* nocapture readnone %this, %class.B* dereferenceable(4) %p1) #0 align 2 {
+entry:
+  %call.i = tail call %class.A* @_ZN1B5m_fn2Ev(%class.B* nonnull %p1)
+  %resize_I.i = getelementptr inbounds %class.B, %class.B* %p1, i32 0, i32 0
+  %0 = load %class.A*, %class.A** %resize_I.i, align 4, !tbaa !3
+  %cmp.6.i = icmp eq %class.A* %0, %call.i
+  br i1 %cmp.6.i, label %_ZN1B5m_fn1Ev.exit, label %for.body.lr.ph.i
+
+for.body.lr.ph.i:                                 ; preds = %entry
+  %resize_I.promoted8.i = ptrtoint %class.A* %0 to i32
+  %scevgep.i = getelementptr %class.A, %class.A* %call.i, i32 -1, i32 0
+  %1 = ptrtoint i8** %scevgep.i to i32
+  %2 = sub i32 %1, %resize_I.promoted8.i
+  br label %for.body.i
+
+for.cond.for.cond.cleanup_crit_edge.i:            ; preds = %for.body.i
+  %3 = lshr i32 %2, 3
+  %4 = add nuw nsw i32 %3, 1
+  %scevgep10.i = getelementptr %class.A, %class.A* %0, i32 %4
+  store %class.A* %scevgep10.i, %class.A** %resize_I.i, align 4, !tbaa !3
+  br label %_ZN1B5m_fn1Ev.exit
+
+for.body.i:                                       ; preds = %for.body.i, %for.body.lr.ph.i
+  %5 = phi %class.A* [ %0, %for.body.lr.ph.i ], [ %incdec.ptr.i, %for.body.i ]
+  %Data.i.i = getelementptr inbounds %class.A, %class.A* %5, i32 0, i32 0
+  store i8* null, i8** %Data.i.i, align 4, !tbaa !8
+  %Length.i.i = getelementptr inbounds %class.A, %class.A* %5, i32 0, i32 1
+  store i32 0, i32* %Length.i.i, align 4, !tbaa !11
+  %incdec.ptr.i = getelementptr inbounds %class.A, %class.A* %5, i32 1
+  %cmp.i = icmp eq %class.A* %incdec.ptr.i, %call.i
+  br i1 %cmp.i, label %for.cond.for.cond.cleanup_crit_edge.i, label %for.body.i
+
+_ZN1B5m_fn1Ev.exit:                               ; preds = %for.cond.for.cond.cleanup_crit_edge.i, %entry
+  ret void
+}
+
+declare %class.A* @_ZN1B5m_fn2Ev(%class.B*) #0
+
+attributes #0 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a8" "target-features"="+neon,+vfp3" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.module.flags = !{!0, !1}
+!llvm.ident = !{!2}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 1, !"min_enum_size", i32 4}
+!2 = !{!"clang version 3.8.0 (trunk 246510) (llvm/trunk 246509)"}
+!3 = !{!4, !5, i64 0}
+!4 = !{!"_ZTS1B", !5, i64 0}
+!5 = !{!"any pointer", !6, i64 0}
+!6 = !{!"omnipotent char", !7, i64 0}
+!7 = !{!"Simple C/C++ TBAA"}
+!8 = !{!9, !5, i64 0}
+!9 = !{!"_ZTS1A", !5, i64 0, !10, i64 4}
+!10 = !{!"int", !6, i64 0}
+!11 = !{!9, !10, i64 4}