[PATCH] D100257: [VPlan] Add VPUserID to distinguish between recipes and others.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:685
+  static inline bool classof(const VPUser *U) {
+    return U->getVPUserID() == VPUser::VPUserID::Recipe;
+  }
----------------
gilr wrote:
> a.elovikov wrote:
> > fhahn wrote:
> > > a.elovikov wrote:
> > > > Can we make `getVPUserID()` protected instead of friendship above?
> > > Unfortunately I don't think so. If it's protected, we can access it from inside a VPRecipeBase object, but here we need to access it from a `VPUser` pointer directly.
> > I'd rather make the enum/method public then, but it's very subjective.
> > I'd rather make the enum/method public then, but it's very subjective.
> 
> +1
I don't mind either way, I made it public. But this has the dis-advantage that the IDs themselves are exposed to users, which should never use them. Probably not a very big deal though.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:276
 
+static_assert(sizeof(VPUser) <= 48, "size of VPUser changed");
+
----------------
gilr wrote:
> fhahn wrote:
> > a.elovikov wrote:
> > > 1) Why is that important? Do we rely on it or is it simply to prevent uncontrolled growth?
> > > 2) Why do we expect it to be the same on 32/64-bit platforms? Don't we have any pointers inside the object? I'd expect the vtable itself would make it different...
> > > Why is that important? Do we rely on it or is it simply to prevent uncontrolled growth?
> > 
> > It's just to prevent accidental growth, but it is not strictly necessary (and we don't have static asserts for other sizes yet), but it is a common pattern, e.g. in various key classes in Clang to prevent unnoticed size growth.
> > 
> > > Why do we expect it to be the same on 32/64-bit platforms?
> > 
> > I think `48` should be the upper bound on 64 bit platforms (unless there are differences caused by different 64 bit operating systems) and on 32-bit platforms it should be less than or equal.
> This seems a bit premature. Perhaps better to have this discussion separate of this patch.
It is just an extra guard which make unintended size changes visible (similar to other places in LLVM/Clang), but it can be a separate patch. I removed it for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100257