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

[Huihui Zhang via Phabricator via llvm-commits]

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