[PATCH] D98169: [IR] Permit load/store/alloca for struct with the same scalable vectors.

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:630
 public:
-  uint64_t getSizeInBytes() const { return StructSize; }
+  uint64_t getSizeInBytes() const { return StructSize.getKnownMinValue(); }
 
----------------
HsiangKai wrote:
> david-arm wrote:
> > This should also return a TypeSize to be consistent with getSizeInBits.
> How about keep the interface as so.
> 
> In current implementation, the elements will be scalar or scalable. The uses of StructLayout::getSizeInBytes() will be compared with StructLayout::getElementOffset() usually. Keep the interface to return uint64_t should be fine.
> 
> I added a FIXME comment before the function.
But isn't the problem here that the result is now wrong? We're now returning the minimum value, rather than a TypeSize that tries to express the complete value. Also, it seems quite confusing that returning a size in bits gives you TypeSize and returning a size in bytes gives you uint64_t. This is inconsistent with how sizes are returned elsewhere, for example see:

  include/llvm/CodeGen/ValueTypes.h

where we have

  TypeSize getSizeInBits() const
  TypeSize getStoreSize() const

where the latter returns bytes. If a function must return the minimum value then I think at least the function name should reflect that with a rename, i.e. getMinSizeInBytes() so that the caller understands this is not the actual size.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98169/new/

https://reviews.llvm.org/D98169