This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Fix usage of DataLayout::getTypeStoreSize()
ClosedPublic

Authored by huihuiz on Mar 19 2020, 1:55 PM.

Details

Summary

DataLayout::getTypeStoreSize() returns TypeSize.

For cases where it can not be scalable vector (e.g., GlobalVariable),
explicitly call TypeSize::getFixedSize().

For cases where scalable property doesn't matter, (e.g., check for
zero-sized type), use TypeSize::isNonZero().

Diff Detail

Event Timeline

huihuiz created this revision.Mar 19 2020, 1:55 PM
efriedma added inline comments.Mar 19 2020, 6:10 PM
llvm/lib/Analysis/ValueTracking.cpp
3499–3500

I'm a little uncomfortable with the way getKnownMinSize() is being used here; it's correct, but I don't really like leaving around a variable deceptively named "Size" in a big function like this.

Maybe it would make sense to add a method isNonZero() to TypeSize?

llvm/test/Transforms/MemCpyOpt/vscale.ll
5 ↗(On Diff #251468)

We don't really need the text of the warning here...

I'm not really happy with the way the test here is exercising this codepath; if the code structure changes, it might end up covering nothing. Can you add a positive test that we detect <vscale x 4 x i32> zeroinitializer as a "bytewise" value?

sdesmalen added inline comments.Mar 20 2020, 10:34 AM
llvm/lib/Analysis/ValueTracking.cpp
3499–3500

That's a good suggestion, there are more instances where such a method would be useful.

huihuiz updated this revision to Diff 251769.Mar 20 2020, 2:37 PM
huihuiz marked 3 inline comments as done.
huihuiz edited the summary of this revision. (Show Details)

Address review comments.

llvm/lib/Analysis/ValueTracking.cpp
3499–3500

Thanks Eli for the suggestion!
Get rid of "Size" in the update, and add TypeSize::isNonZero() to use here.

3499–3500

There are quite a few places, will post a nfc patch for refactoring.

llvm/test/Transforms/MemCpyOpt/vscale.ll
5 ↗(On Diff #251468)

I am updating this test with more zero/non-zero array/struct to check for isBytewiseValue.
For vector, zero element is illegal. So only test for non-zero for vector. But single store instruction will help hit isBytewiseValue codepath.

huihuiz updated this revision to Diff 251778.Mar 20 2020, 2:57 PM
This revision is now accepted and ready to land.Mar 20 2020, 3:16 PM
This revision was automatically updated to reflect the committed changes.