This is an archive of the discontinued LLVM Phabricator instance.

[NFC][llvm] Make the constructors of `ElementCount` private.
ClosedPublic

Authored by fpetrogalli on Aug 17 2020, 4:11 PM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Aug 17 2020, 4:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 4:11 PM
fpetrogalli requested review of this revision.Aug 17 2020, 4:11 PM
vkmr added a subscriber: vkmr.Aug 18 2020, 4:36 AM
ctetreau added inline comments.Aug 18 2020, 9:22 AM
llvm/include/llvm/IR/Intrinsics.h
137–139

use getFixed

llvm/include/llvm/Support/TypeSize.h
45–46

since this is now private, and nothing uses it, it should be deleted

llvm/lib/CodeGen/ValueTypes.cpp
52

this can just pass the input ElementCount to VectorType::get

fpetrogalli marked 3 inline comments as done.

Thank you for the code review @ctetreau.

Francesco

llvm/include/llvm/Support/TypeSize.h
45–46

I have anyway marked the default constructor as deleted. I have explained why in a comment in the sources.

ctetreau accepted this revision.Aug 18 2020, 10:14 AM

Fix the typo, then this looks good to me.

llvm/include/llvm/IR/Intrinsics.h
137–139

NIT: TYPO: "There si no" -> "There is no"

This revision is now accepted and ready to land.Aug 18 2020, 10:14 AM
fpetrogalli marked an inline comment as done.Aug 18 2020, 11:59 AM
david-arm added inline comments.Aug 19 2020, 12:25 AM
llvm/lib/IR/ConstantFold.cpp
922–923

Just a thought for code like this - since we're now calling ElementCount::get that means we can now use the 'auto' keyword in such places. This might help reduce the character count and help to fit on one line?

fpetrogalli marked an inline comment as done.

Thank you for the review @david-arm.

llvm/lib/IR/ConstantFold.cpp
922–923

Good point! Done.

fpetrogalli retitled this revision from [NFC][llvm] Make the contructors of `ElementCount` private. to [NFC][llvm] Make the constructors of `ElementCount` private..Aug 19 2020, 9:04 AM
This revision was landed with ongoing or failed builds.Aug 19 2020, 9:27 AM
This revision was automatically updated to reflect the committed changes.
ftynse added a subscriber: ftynse.Aug 19 2020, 9:44 AM

FYI, this broke MLIR build.

@ftynse @mehdi_amini @JDevlieghere - apologies for breaking your workflows, and thank you for acting so quickly. I started doing the revert myself as soon as I saw the bombing from buildbot but by the time I was ready to push my revert your fixes were already pouring in.

I believe this can now be considered resolved. The patch from @mehdi_amini at https://github.com/llvm/llvm-project/commit/a407ec9b6db1e29e9aa361819f499ad11038d2dd is making sure that all subprojects are happy.

Thank you for your help.

Francesco

Why were static "get" methods preferred over simply marking the constructor explicit? For example, if the constructor is marked explicit, then this is what users see:

    llvm::ElementCount EC = {vector->getNumElements(), /*scalable*/ false};
                       ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/dave/s/n/llvm/include/llvm/Support/TypeSize.h:37:12: note: explicit constructor declared here
  explicit ElementCount(unsigned Min, bool Scalable) : Min(Min), Scalable(Scalable) {}
           ^
1 error generated.

Why were static "get" methods preferred over simply marking the constructor explicit? For example, if the constructor is marked explicit, then this is what users see:

    llvm::ElementCount EC = {vector->getNumElements(), /*scalable*/ false};
                       ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/dave/s/n/llvm/include/llvm/Support/TypeSize.h:37:12: note: explicit constructor declared here
  explicit ElementCount(unsigned Min, bool Scalable) : Min(Min), Scalable(Scalable) {}
           ^
1 error generated.

Hi @davezarzycki ,

thank you for your comment. The idea of introducing get* methods was to make sure the users would make a conscious decision about whether they where setting up Fixed o Scalable instances of ElementCount. As for the general constructor you mention, we could have 1. allowed people to use initializer lists for ElementCount or alternatively 2. mark the constructor as explicit. With @ctetreau we decided to have an uniform approach, so we decided to allow building instances of ElementCount only via static methods. So, no real technical reason for not using "explicit", other than our preference.

Kind regards,

Francesco