[PATCH] D76454: [ValueTracking] Fix usage of DataLayout::getTypeStoreSize()
Huihui Zhang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 20 14:40:14 PDT 2020
huihuiz added inline comments.
================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3500
- const uint64_t Size = DL.getTypeStoreSize(V->getType());
+ const uint64_t Size = DL.getTypeStoreSize(V->getType()).getKnownMinSize();
if (!Size)
----------------
sdesmalen wrote:
> efriedma wrote:
> > 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?
> That's a good suggestion, there are more instances where such a method would be useful.
Thanks Eli for the suggestion!
Get rid of "Size" in the update, and add TypeSize::isNonZero() to use here.
================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3500
- const uint64_t Size = DL.getTypeStoreSize(V->getType());
+ const uint64_t Size = DL.getTypeStoreSize(V->getType()).getKnownMinSize();
if (!Size)
----------------
huihuiz wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > 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?
> > That's a good suggestion, there are more instances where such a method would be useful.
> 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.
================
Comment at: llvm/test/Transforms/MemCpyOpt/vscale.ll:5
+; Check that this test doesn't cause compier warning
+; warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
+
----------------
efriedma wrote:
> 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?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76454/new/
https://reviews.llvm.org/D76454
More information about the llvm-commits
mailing list