rovka accepted this revision.
rovka marked 2 inline comments as done.
rovka added a comment.
This revision is now accepted and ready to land.
This looks good to me, maybe wait a while to see if anyone else has any further
comments.
================
Comment at: llvm/include/llvm/IR/DataLayout.h:454
+ auto BaseSize = getTypeSizeInBits(Ty);
+ return { (BaseSize.getKnownMinSize() + 7) / 8, BaseSize.isScalable() };
}
----------------
huntergr wrote:
> rovka wrote:
> > We already overload operator /, why not overload + as well so we don't have
> > to change the body of this method?
> Scaling a size with * or / has a clear meaning to me, since it's independent
> of vscale; getting a vector that's half the size or four times larger just
> works.
>
> Using + (or -) on the other hand doesn't seem to be as clear; I wasn't sure
> if a standalone int should be automatically treated as being the same as the
> TypeSize, or always considered Fixed. If we try for the former I can imagine
> quite a few bugs arising.
>
> I could add a roundBitsToNearestByteSize method to move the arithmetic
> elsewhere if that would be acceptable?
You're right, + on TypeSizes would be confusing. This looks ok as-is then, no
need to fiddle with it more.
================
Comment at: llvm/include/llvm/IR/DataLayout.h:656
+
getTypeSizeInBits(VTy->getElementType()).getKnownMinSize();
+ return ScalableSize(MinBits, EltCnt.Scalable);
}
----------------
huntergr wrote:
> rovka wrote:
> > Maybe just return VTy->getElementCount() *
> > getTypeSizeInBits(VTy->getElementType()).getFixedSize().
> There's no support for generating a TypeSize from an ElementCount in that
> way; is that an interface you feel is useful?
>
> (I'll certainly change the `getKnownMinSize` to `getFixedSize` though, since
> we're just referring to a scalar)
Actually, no, now that I think about it a bit more it might be clearer to spell
it out this way.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:122
+ // Use in places where a scalable size doesn't make sense (e.g. non-vector
+ // types, or vectors in backends which don't support scalable vectors)
+ uint64_t getFixedSize() const {
----------------
Microscopic nit: punctuation.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:143
+ // NOTE: This interface is obsolete and will be removed in a future version
+ // of LLVM in favour of calling getFixedSize() directly
+ operator uint64_t() const {
----------------
Ditto.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:148
+
+ // Additional convenience operators needed to avoid ambiguous parses
+ // TODO: Make uint64_t the default operator?
----------------
Ditto.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53137/new/
https://reviews.llvm.org/D53137
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
