This is an archive of the discontinued LLVM Phabricator instance.

Create a getelementptr instead of sub expr for ValueOffsetPair if the value is a pointer
ClosedPublic

Authored by wmi on Aug 31 2016, 9:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 69871.Aug 31 2016, 9:56 AM
wmi retitled this revision from to Create a getelementptr instead of sub expr for ValueOffsetPair if the value is a pointer.
wmi updated this object.
wmi added a reviewer: sanjoy.
wmi set the repository for this revision to rL LLVM.
wmi added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Sep 9 2016, 4:45 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Analysis/ScalarEvolutionExpander.cpp
1711

I'm not sure it is correct to use getScalarSizeInBits here -- what if ETy is i16 * on a 64 bit machine? Won't getScalarSizeInBits return 16 when it should have returned 64? I think you need to use ScalarEvolution::getTypeSizeInBits.

Secondly, you also need to handle the case when Offset is not divisible by the byte size of ETy (that is, say offset is 6 bytes and ETy is an i32). In that case you'll need to cast V to an i8*, do a byte gep on that value, and then cast the result back to V->getType().

This revision now requires changes to proceed.Sep 9 2016, 4:45 PM
wmi added inline comments.Sep 9 2016, 5:24 PM
lib/Analysis/ScalarEvolutionExpander.cpp
1711

Nice catches. I think you are right for both issues. Will fix them.

wmi updated this revision to Diff 70971.Sep 11 2016, 9:10 PM
wmi edited edge metadata.

Address Sanjoy's comments. I wanted to find a testcase to test the case that the offset is not divisible by the byte size of ETy but didn't succeed, so I forced the code to go through that branch and eyeball the output.

I wanted to find a testcase to test the case that the offset is not divisible by the byte size of ETy but didn't succeed, so I forced the code to go through that branch and eyeball the output.

Have you tried writing a C++ test case (in unittests/Analysis)?

lib/Analysis/ScalarEvolutionExpander.cpp
1708

More idiomatic is

if (PointerType *VTy = dyn_cast<PointerType>(V->getType())) {
}
1714

s/SE.getTypeSizeInBits(Ety)/ESize/

Also, for sanity, I'd suggest making the association on -Offset * 8 / SE.getTypeSizeInBits(Ety) a 100% obvious (i.e. -(Offset * 8) / ESize).

1722

In this case I'd s/"scevgep"/"uglygep"/: http://llvm.org/docs/GetElementPtr.html#what-s-an-uglygep

sanjoy requested changes to this revision.Sep 11 2016, 9:17 PM
sanjoy edited edge metadata.

.

This revision now requires changes to proceed.Sep 11 2016, 9:17 PM
wmi added a comment.Sep 11 2016, 9:34 PM

I didn't try writing a C++ test case. What I tried was to add an assertion in the branch and run a lot of C/C++ benchmarks but I didn't catch such testcase. What's your idea about writting a C++ testcase?

lib/Analysis/ScalarEvolutionExpander.cpp
1708

Fixed.

1714

Fixed.

1722

Fixed.

wmi updated this revision to Diff 70972.Sep 11 2016, 9:35 PM
wmi edited edge metadata.

What's your idea about writting a C++ testcase?

What's your idea about writting a C++ testcase?

Sorry, fat fingered the previous response. By a C++ test case, I meant writing some C++ code that makes the right calls to getSCEV and SCEVExpander::expand to trigger the bad behavior. I'm not sure if that will be easy or even possible, but it is worth a try.

wmi added a comment.Sep 11 2016, 9:49 PM

I never wrote such unittest before, but I just looked at unittests/Analysis/ScalarEvolutionTest.cpp and felt it was indeed more promising to achieve an expected testcase than writing .ll unitest. I will give it a shot. Thanks for the good suggestion.

This revision was automatically updated to reflect the committed changes.
wmi updated this revision to Diff 71398.Sep 14 2016, 11:02 AM
wmi edited edge metadata.

Create a C++ unittest to exercise the branch where the offset is not divisible by the elem type size of value.

lgtm with some comments inline.

unittests/Analysis/ScalarEvolutionTest.cpp
281 ↗(On Diff #71398)

You don't need to "manually" create an llvm::Function here. Take a look at how other C++ tests use parseAssemblyString and instruction names to avoid this kind of boilerplate.

Having said that, I don't really mind what you have here, it is just under the level of complexity where using parseAssemblyString would pay off.

293 ↗(On Diff #71398)

s/bitcase/bitcast/

302 ↗(On Diff #71398)

LLVM style is CastA and CastB.

327 ↗(On Diff #71398)

Use EXPECT_TRUE(isa<BitCastInst>(...)) here.

wmi added inline comments.Sep 14 2016, 1:26 PM
unittests/Analysis/ScalarEvolutionTest.cpp
281 ↗(On Diff #71398)

I manually create the function to be consistent with other func in the file and the complexity is not too high. I admit using parseAssemblyString will be simpler, but since it doesn't affect the readability much I tend to be lazy and keep it.

293 ↗(On Diff #71398)

Fixed.

302 ↗(On Diff #71398)

Ok, fixed.

327 ↗(On Diff #71398)

Fixed.