Page MenuHomePhabricator

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

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



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

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?

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

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.


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


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

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.