[PATCH] D88409: [SVE] Make ElementCount and TypeSize use a new PolySize class
Christopher Tetreault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 29 07:56:03 PDT 2020
ctetreau added inline comments.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:38
+public:
+ PolySize() = default;
----------------
david-arm wrote:
> ctetreau wrote:
> > david-arm wrote:
> > > ctetreau wrote:
> > > > Why does there need to be a default constructor?
> > > I think because there are places in the code base that rely upon it - this isn't something new as it was already in ElementCount. I think there is one example in llvm/include/llvm/IR/Intrinsics.h where the ElementCount is a member of a union. I'm pretty sure that removing the default constructor causes compilation errors, but I can try again to make sure.
> > Assuming it's necessary to have this constructor, what should it do? Should it explicitly be defined to be {0, false}? Is it a garbage value?
> >
> > I suppose {0, false} sort of is already a garbage value, so maybe everything can just assert that it's not this?
> I think it's just like someone declaring a variable and then initialising it later, no different from a normal integer type that has a default constructor, i.e.
>
> uint64_t SomeVal;
> ...
> SomeVal = name == "Bob" ? 3 : 7;
>
> If we want to remove the default constructor we could do it, but it will require some work to fix up cases where it's relied upon. I can certainly see what breaks when I remove it?
I guess it might be nice to be able to do this. When ElementCount and TypeSize were just PODs, I was more OK with expecting the user to not mess it up, but since we're making them proper classes now...
I think if you make isScalable() and getKnownMinValue() assert that it's not {0, false}, then we can leave the default constructor for ergonomic purposes. If everything else were implemented in terms of these functions, then everything would be covered by the assert.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88409/new/
https://reviews.llvm.org/D88409
More information about the llvm-commits
mailing list