This is an archive of the discontinued LLVM Phabricator instance.

Fix vector splitting for extract_vector_elt and vector elements of <8-bits.
ClosedPublic

Authored by dsanders on Sep 3 2015, 2:07 AM.

Details

Summary

One of the vector splitting paths for extract_vector_elt tries to lower:

define i1 @via_stack_bug(i8 signext %idx) {
  %1 = extractelement <2 x i1> <i1 false, i1 true>, i8 %idx
  ret i1 %1
}

to:

define i1 @via_stack_bug(i8 signext %idx) {
  %base = alloca <2 x i1>
  store <2 x i1> <i1 false, i1 true>, <2 x i1>* %base
  %2 = getelementptr <2 x i1>, <2 x i1>* %base, i32 %idx
  %3 = load i1, i1* %2
  ret i1 %3
}

However, the elements of <2 x i1> are not byte-addressible. The result of this
is that the getelementptr expands to '%base + %idx * (1 / 8)' which simplifies
to '%base + %idx * 0', and then simply '%base' causing all values of %idx to
extract element zero.

This commit fixes this by promoting the vector elements of <8-bits to i8 before
splitting the vector.

This fixes a number of test failures in pocl.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 33914.Sep 3 2015, 2:07 AM
dsanders retitled this revision from to Fix vector splitting for extract_vector_elt and vector elements of <8-bits..
dsanders updated this object.

The fix looks good, but maybe the test should be an .ir test as this probably affects other targets than MIPS too? Also could be a candidate for 3.7.1?

The fix looks good, but maybe the test should be an .ir test as this probably affects other targets than MIPS too?

It can affect other targets but we can't make a generic test because the legalization settings for the target determine whether the bug is encountered. If v2i1 is legal or the legalizer widens/promotes the vector instead of splitting it, the bug won't occur.

Also could be a candidate for 3.7.1?

Yes, I'm planning to nominate it for 3.7.1 once it's committed.

This revision is now accepted and ready to land.Sep 8 2015, 1:23 PM
dsanders closed this revision.Sep 9 2015, 2:54 AM
This revision was automatically updated to reflect the committed changes.