This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Avoid casting a vector of size less than 8 bits to i8
ClosedPublic

Authored by ormris on Jun 19 2018, 11:43 AM.

Details

Summary

A reprise of D25849.

This crash was found through fuzzing some time ago and was documented in PR28879.

No check for load size has been added due to the following tests:

  • Transforms/GVN/invariant.group.ll
  • Transforms/GVN/pr10820.ll

These tests expect load sizes that are not a multiple of eight.

Thanks to @davide for the original patch.

Diff Detail

Event Timeline

ormris created this revision.Jun 19 2018, 11:43 AM

It should be possible to implement this sort of coercion with an appropriate cast, rather than just bailing out, I think. But I won't block the patch on that.

Please add a testcase for the load-load case, in addition to the load-store case.

RKSimon added inline comments.Jun 20 2018, 9:12 AM
test/Transforms/GVN/pr28879.ll
2

FileCheck? I realise its a crash test but some sanity checks could be helpful

@efriedma I'm not quite sure what that would look like here. Are you looking for something like the following?

%a = alloca <7 x i1>, align 2
store <7 x i1> undef, <7 x i1>* %a, align 2
%0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
%val0 = load i1, i1* %0, align 2
%val1 = load i1, i1* %0, align 2
br i1 %val0, label %cond.true, label %cond.false
test/Transforms/GVN/pr28879.ll
2

Agreed. I'll take a look at it.

For the test, I mean something like the following:

define <7 x i1> @f(<7 x i1>* %a) {
entry:
  %vec = load <7 x i1>, <7 x i1>* %a
  %0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
  %val = load i1, i1* %0, align 2
  br i1 %val, label %cond.true, label %cond.false

cond.true:
  ret <7 x i1> %vec

cond.false:
  ret <7 x i1> <i1 false, i1 false, i1 false, i1 false, i1 false, i1 false, i1 false>
}
ormris updated this revision to Diff 152364.Jun 21 2018, 1:07 PM

Add load-load case and sanity checks

efriedma added inline comments.Jun 21 2018, 1:30 PM
lib/Transforms/Utils/VNCoercion.cpp
25

Please add a brief comment here explaining why we bail out.

test/Transforms/GVN/load-load-odd-vector.ll
3 ↗(On Diff #152364)

Please put this test into the same file as the other test.

ormris updated this revision to Diff 152376.Jun 21 2018, 1:57 PM

Merge two tests. Add comment.

This revision is now accepted and ready to land.Jun 21 2018, 2:01 PM

Thanks for the review @efriedma and @RKSimon! I'll go ahead and commit this.

This revision was automatically updated to reflect the committed changes.