Page MenuHomePhabricator

canCreateUndefOrPoison: dyn_cast -> dyn_cast_or_null
ClosedPublic

Authored by markus on Jan 12 2021, 6:51 AM.

Details

Summary

Trivial change of a dyn_cast to dyn_cast_or_null in ValueTracking.

Diff Detail

Event Timeline

markus created this revision.Jan 12 2021, 6:51 AM
markus requested review of this revision.Jan 12 2021, 6:51 AM

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

llvm/lib/Analysis/ValueTracking.cpp
4740

Constant::getAggregateElement() at line 4733 may return nullptr.

markus edited the summary of this revision. (Show Details)Jan 12 2021, 6:57 AM
markus added a reviewer: aqjune.
fhahn added a subscriber: fhahn.Jan 12 2021, 8:20 AM

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

Yes, please add a regression test.

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

Probably just in its own file (we don't do a very good job of keeping a lot of related tests in a single file together - usually just add more files, it's not ideal but often hard to find other good groupings) - but if you're looking for related test files, try breaking the pass in some obvious way and see what tests fail - that should help you find some related tests that could be piggy backed on.

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

Probably just in its own file (we don't do a very good job of keeping a lot of related tests in a single file together - usually just add more files, it's not ideal but often hard to find other good groupings) - but if you're looking for related test files, try breaking the pass in some obvious way

Such as adding assert(false) in the codepath you're experimenting with, etc.

and see what tests fail - that should help you find some related tests that could be piggy backed on.

Thanks!
Yes, a test should be added.
I guess

ashr <4 x i16> %induction, select (i1 icmp sgt (i16 ptrtoint (i16* @c to i16), i16 1), <4 x i16> zeroinitializer, <4 x i16> <i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16)>)

is the problematic part?
If it is non-trivial to further reduce the input, you can also add this instruction to the list at ValueTrackingTest.cpp 's TEST(ValueTracking, canCreatePoisonOrUndef).

markus updated this revision to Diff 316343.Jan 13 2021, 1:23 AM

Added test.

Thanks!
Yes, a test should be added.
I guess

ashr <4 x i16> %induction, select (i1 icmp sgt (i16 ptrtoint (i16* @c to i16), i16 1), <4 x i16> zeroinitializer, <4 x i16> <i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16)>)

is the problematic part?
If it is non-trivial to further reduce the input, you can also add this instruction to the list at ValueTrackingTest.cpp 's TEST(ValueTracking, canCreatePoisonOrUndef).

Yes that is the problematic part. I think it makes most sense to add as a targeted unit-test for the specific function as below. Unfortunately the IR line is a bit messy but I was not able to simplify it any further. I guess it needs to be something that is considered a Constant but yet does not simplify to an "actual" Constant during import.

aqjune accepted this revision.Jan 13 2021, 3:48 AM

LGTM

This revision is now accepted and ready to land.Jan 13 2021, 3:48 AM
This revision was automatically updated to reflect the committed changes.