[PATCH] D88409: [SVE] Make ElementCount and TypeSize use a new PolySize class

David Sherwood via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 6 00:09:01 PDT 2020


david-arm added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:38
+public:
+  PolySize() = default;
 
----------------
ctetreau wrote:
> david-arm wrote:
> > ctetreau wrote:
> > > 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.
> > I tried replacing "PolySize() = default" with something explicit like "PolySize() : MinVal(0), IsScalable(false) {}", but sadly the compiler says this is not good enough for IITDescriptor. Since we can't be sure exactly what the default value is (I assume it's {0,false} but perhaps someone more knowledgeable can confirm?) I'd have to add asserts in isScalable, getKnownMinValue, getFixedSize and so on along the lines of:
> > 
> > assert(*this != PolySize())
> I'm surprised that `PolySize() : MinVal(0), IsScalable(false) {}` doesn't work, what error does it give? Some issue with assigning 0 to the template field?
> 
> Regardless, I think you can assign values at the field headers, and it will be taken as the default. So do:
> 
> ```
>   T MinVal = 0;            // The minimum value that it could be.
>   bool IsScalable = false; // If true, the total value is determined by multiplying
> ```
Hi @ctetreau, so having that constructor is fine, it's just that the compilation of IITDescriptor still fails with the same complaint about a lack of default constructors. :( Sometimes I feel C++ can be incredibly powerful, sometimes it just seems to thwart us at every corner! I'll try your suggestion of assigning default values.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:45
   }
 
   /// Counting predicates.
----------------
vkmr wrote:
> ctetreau wrote:
> > vkmr wrote:
> > > It might be useful to add an explicit `getNone()` (or `getZero()`) function that simply returns `{0, false}` value. For instance, there are places using `Optional<ElementCount>`for VF  (and `Optional<unsigned>` where VF is not yet ported to `ElementCount`) and use `None` to represent VectorizationFailure.  We already have `isZero()` and `isNonZero()` functions so a Zero getter should not be out of tune.
> > Bikeshedding: Maybe it should be named something like `getNull()` or `getInvalid()`? You propose using `getZero()` instead of having an `Optional<ElementCount>`. I think this usage is fine, but we should be clear that the thing is a garbage value, not zero in the mathematical sense. This lets us make the garbage value be something different in the future if there ends up being a better option.
> Good point @ctetreau! I am not too keen on one name over other, but if I have to, my vote is for `getNull()`; I feel it best conveys the idea. 
Thanks for the suggestion @ctetreau, I'll change the name to getNull()!


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

https://reviews.llvm.org/D88409



More information about the llvm-commits mailing list