[PATCH] D140263: [NFC] Vastly simplifies TypeSize

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 8 inline comments as done.
gchatelet added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:42
+/// to represent stack offsets.
+class StackOffset {
+  int64_t Fixed = 0;
----------------
courbet wrote:
> Given that this is not a template over its unit, I think the doc should explicitly mention that the unit is bytes.
I've reworded. Let me know if it's better.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:168
 
-  /// Add \p RHS to the value at the univariate dimension.
-  LeafTy getWithIncrement(ScalarTy RHS) const {
-    return static_cast<LeafTy>(
-        UnivariateLinearPolyBase(Value + RHS, UnivariateDim));
+  /// Add \p Rhs to the value at the univariate dimension.
+  LeafTy getWithIncrement(ScalarTy Rhs) const {
----------------
barannikov88 wrote:
> gchatelet wrote:
> > gchatelet wrote:
> > > barannikov88 wrote:
> > > > gchatelet wrote:
> > > > > barannikov88 wrote:
> > > > > > univariate dimension --> fixed quantity?
> > > > > > 
> > > > > Reworded it MinValue since this is what it is.
> > > > Thanks. However, MinValue is an implementation detail that should not be exposed even in comments.
> > > > Thanks. However, MinValue is an implementation detail that should not be exposed even in comments.
> > > 
> > > Is that true ? I'm under the impression that this is what it was intended for in the original patch
> > > https://github.com/llvm/llvm-project/commit/b302561b763a1d2eb1a450e135b8d49931936755
> > > 
> > > ```
> > > * Adds a TypeSize struct to represent the known minimum size of a type
> > >   along with a flag to indicate that the runtime size is a integer multiple
> > >   of that size
> > > ```
> > Let me rephrase "yes you're right that this is an implementation detail and it shouldn't be exposed but in that particular case it seems to be the exact semantic for the type".
> > I think it's better to update the documentation to reflect what was in the original comment. WDYT?
> Sorry for not being clear clear.
> I meant that "MinValue" (spelled exactly) is a private field, which users of the class should not care about. "Minimum value", e.g., is different.
> Since this class is now abstract, I'd avoid using "Min" in variable name and "Minimum" in the comment.
> Finally, I'd remove this method and add `operator+` instead :) There is an (outdated?) comment near divideCoefficientBy that suggests of not doing it though.
> 
> Sorry for not being clear clear.
> I meant that "MinValue" (spelled exactly) is a private field, which users of the class should not care about. "Minimum value", e.g., is different.
> Since this class is now abstract, I'd avoid using "Min" in variable name and "Minimum" in the comment.

Sounds good to me, I went over the file and tried to use appropriate variable names where possible.

> Finally, I'd remove this method and add `operator+` instead :) There is an (outdated?) comment near divideCoefficientBy that suggests of not doing it though.

I think the comment refers to the class hierarchy that was in use before this patch.
I don't think there is a need for it anymore. It seems clear enough to me what the classical operators would mean for `ElementCount` and `TypeSize` without using the long names.

That said, and if you don't mind I'd rather do this in a second patch and keep this one NFC. There's already quite a few things going on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140263