This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Vastly simplifies TypeSize
ClosedPublic

Authored by gchatelet on Dec 17 2022, 9:53 AM.

Details

Summary

Simplifies the implementation of TypeSize while retaining its interface.
There is no need for abstract concepts like LinearPolyBase, UnivariateLinearPolyBase or LinearPolySize.

Diff Detail

Event Timeline

gchatelet created this revision.Dec 17 2022, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2022, 9:53 AM
gchatelet requested review of this revision.Dec 17 2022, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2022, 9:53 AM

I like this patch as now I almost understand what these three classes are. But please see the comments.

llvm/include/llvm/Support/TypeSize.h
87

Would it be better to move the below comments to the derived classes? It is not immediately clear that the arrows represent relationship between classes.

105

LHS and RHS (all capital) seems canonical abbreviation (892 occurences of LHS, 16 occurences of Lhs in CodeGen). Retaining the names would also make the diff smaller and make it a lot easier to review.

142

Personally, this looks redundant to me (saving a few characters?).

145–146

univariate dimension --> fixed quantity?

gchatelet updated this revision to Diff 483818.Dec 18 2022, 6:28 AM
  • Make Rhs/Lhe uppercase
gchatelet updated this revision to Diff 483819.Dec 18 2022, 6:41 AM
gchatelet marked 3 inline comments as done.
  • Remove tie and reword description

Thx @barannikov88 for the comments. This patch is mostly to better understand what purpose TypeSize serves.
I'm trying to add a set of useful basic types D128052 and ElementCount / TypeSize will probably be tied to one of these at some point.

llvm/include/llvm/Support/TypeSize.h
145–146

Reworded it MinValue since this is what it is.

Mostly fine by me, but I think the initial author should take a look.

llvm/include/llvm/Support/TypeSize.h
102–104

Here, and in the method below, the error message is outdated.

145–146

Thanks. However, MinValue is an implementation detail that should not be exposed even in comments.

166–167

This class is no longer "size", it is now "quantity". Either the comments should be fixed, or the class name.

barannikov88 added inline comments.Dec 18 2022, 8:23 AM
llvm/include/llvm/Support/TypeSize.h
92

Looks redundant.

courbet added inline comments.Dec 19 2022, 4:54 AM
llvm/include/llvm/Support/TypeSize.h
42

Given that this is not a template over its unit, I think the doc should explicitly mention that the unit is bytes.

91–92

This is now abstractly named ("Quantity"), but the documentation still refers to "size".

92

Update doc

141–144

Let's take this opportunity to document this non-obvious bool operator.

gchatelet marked 2 inline comments as done.Dec 19 2022, 6:04 AM
gchatelet added inline comments.
llvm/include/llvm/Support/TypeSize.h
145–146

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
gchatelet marked an inline comment as done.Dec 19 2022, 6:07 AM
gchatelet added inline comments.
llvm/include/llvm/Support/TypeSize.h
145–146

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?

barannikov88 added inline comments.Dec 19 2022, 6:57 AM
llvm/include/llvm/Support/TypeSize.h
145–146

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.

gchatelet marked 8 inline comments as done.Dec 19 2022, 8:02 AM
gchatelet added inline comments.
llvm/include/llvm/Support/TypeSize.h
42

I've reworded. Let me know if it's better.

145–146

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.

gchatelet updated this revision to Diff 483969.Dec 19 2022, 8:03 AM
gchatelet marked 2 inline comments as done.
  • Address comments
gchatelet updated this revision to Diff 483970.Dec 19 2022, 8:05 AM
  • rephrased comment
gchatelet updated this revision to Diff 485278.Dec 26 2022, 2:59 AM
  • rebase and mark most functions/ctors constexpr

Ready for another round of comments if needed

Adding the original author (@sdesmalen) to try to understand the purpose of these abstractions.
The original patch (https://reviews.llvm.org/D88982) does not give any insight into why this is helpful.

Adding the original author (@sdesmalen) to try to understand the purpose of these abstractions.
The original patch (https://reviews.llvm.org/D88982) does not give any insight into why this is helpful.

(i personally suspect it was a part of series(?) to support spills to stack of scalable vectors)

The code in this patch looks good to me, and I think it turned easier to read than the LinearPoly* abstractions. So unless there is a good reason to keep them, LGTM.

The initial implementation was indeed too generic and complicated. It followed from discussions where we thought there might be value in keeping it generic so that we might extend it to more dimensions at some point. But given the complexity of adding even just one extra dimension, I don't see this happening any time soon (if ever). The refactor looks like a welcome improvement!

llvm/include/llvm/Support/TypeSize.h
42

Seems fine to me.

Thx @sdesmalen, I'll land this now then.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 6 2023, 5:19 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

This commit breaks the build for me (and some of the buildbots): https://lab.llvm.org/buildbot/#/builders/16/builds/41419/steps/5/logs/stdio

CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DEXPENSIVE_CHECKS -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/1/llvm-clang-x86_64-expensive-checks-debian/build/lib/IR -I/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR -I/b/1/llvm-clang-x86_64-expensive-checks-debian/build/include -I/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/include -U_GLIBCXX_DEBUG -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o -MF lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o.d -o lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o -c /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:73:3: error: no type named 'StructuralHash' in namespace 'llvm::details'; did you mean '::details::StructuralHash'?
  details::StructuralHash H;
  ^~~~~~~~~~~~~~~~~~~~~~~
  ::details::StructuralHash
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:25:7: note: '::details::StructuralHash' declared here
class StructuralHash {
      ^
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:79:3: error: no type named 'StructuralHash' in namespace 'llvm::details'; did you mean '::details::StructuralHash'?
  details::StructuralHash H;
  ^~~~~~~~~~~~~~~~~~~~~~~
  ::details::StructuralHash
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:25:7: note: '::details::StructuralHash' declared here
class StructuralHash {
      ^
2 errors generated.
ABataev added a subscriber: ABataev.Jan 6 2023, 7:26 AM

This commit breaks the build for me (and some of the buildbots): https://lab.llvm.org/buildbot/#/builders/16/builds/41419/steps/5/logs/stdio

CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DEXPENSIVE_CHECKS -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/1/llvm-clang-x86_64-expensive-checks-debian/build/lib/IR -I/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR -I/b/1/llvm-clang-x86_64-expensive-checks-debian/build/include -I/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/include -U_GLIBCXX_DEBUG -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o -MF lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o.d -o lib/IR/CMakeFiles/LLVMCore.dir/StructuralHash.cpp.o -c /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:73:3: error: no type named 'StructuralHash' in namespace 'llvm::details'; did you mean '::details::StructuralHash'?
  details::StructuralHash H;
  ^~~~~~~~~~~~~~~~~~~~~~~
  ::details::StructuralHash
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:25:7: note: '::details::StructuralHash' declared here
class StructuralHash {
      ^
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:79:3: error: no type named 'StructuralHash' in namespace 'llvm::details'; did you mean '::details::StructuralHash'?
  details::StructuralHash H;
  ^~~~~~~~~~~~~~~~~~~~~~~
  ::details::StructuralHash
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/IR/StructuralHash.cpp:25:7: note: '::details::StructuralHash' declared here
class StructuralHash {
      ^
2 errors generated.

+1, please revert the commit.

Reverting now

llvm/include/llvm/Support/TypeSize.h