Page MenuHomePhabricator

Fix IRBuilder CreateBitOrPointerCast for vector types

Authored by sbaranga on Sep 2 2015, 9:33 AM.



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.

Diff Detail


Event Timeline

sbaranga updated this revision to Diff 33818.Sep 2 2015, 9:33 AM
sbaranga retitled this revision from to Fix IRBuilder CreateBitOrPointerCast for vector types.
sbaranga updated this object.
sbaranga added a subscriber: llvm-commits.
jmolloy added a subscriber: jmolloy.Sep 2 2015, 9:42 AM

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

Also, testcase?

rengolin accepted this revision.Sep 2 2015, 2:59 PM
rengolin added a reviewer: rengolin.
rengolin added a subscriber: 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.


This revision is now accepted and ready to land.Sep 2 2015, 2:59 PM
mzolotukhin requested changes to this revision.Sep 2 2015, 2:59 PM
mzolotukhin added a reviewer: mzolotukhin.
mzolotukhin added a subscriber: 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?


1355 ↗(On Diff #33818)

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>)?

This revision now requires changes to proceed.Sep 2 2015, 2:59 PM

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

class A {

char *Data;
int Length;


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

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

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(); }

Thanks, Evgeniy! Here is IR testcase:


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?).


1355 ↗(On Diff #33818)

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 added inline comments.Sep 2 2015, 5:19 PM
1355 ↗(On Diff #33818)

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–1361 ↗(On Diff #33818)

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

sbaranga updated this revision to Diff 33915.Sep 3 2015, 2:16 AM
sbaranga edited edge metadata.

Addressed review comments and added reduced testcase.

sbaranga updated this revision to Diff 33916.Sep 3 2015, 2:24 AM
sbaranga edited edge metadata.

Also add some comments to the test case.

This revision was automatically updated to reflect the committed changes.


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


sbaranga added inline comments.Sep 3 2015, 4:42 AM
1355 ↗(On Diff #33818)

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

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).


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

Further reduced the testcase in r246848.