ctetreau accepted this revision.
ctetreau added a comment.
I think this is good to go as is. Assuming @paulwalker-arm is satisfied with
leaving operator/ as is, then LGTM.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:66
+
+ ElementCount &operator/=(unsigned RHS) {
+ Min /= RHS;
----------------
paulwalker-arm wrote:
> If you add an assert that the divide is lossless (i.e. MIN % RHS == 0) then
> asserts like:
> ```
> assert(EltCnt.isKnownEven() && "Splitting vector, but not in half!");
> ```
> are no longer required. Plus those places which are not checking for
> lossless division will be automatically protected. This feels like a
> sensible default to me. If somebody wants a truncated result, they can do
> the maths using getKnownMinValue().
I would prefer that this not be done. This would make this function non-total
in an unrecoverable way, and would force everybody to write a bunch of tedious
error handling code, even if the normal integer division behavior would have
been fine:
```
ElementCount res = LHS.getKnownMinValue() % RHS.getKnownMinValue() == 0 ? LHS /
RHS : SomeOtherThing;
```
Everybody knows how integer division works, so I think the lossy behavior will
not surprise anybody. An assert might.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86065/new/
https://reviews.llvm.org/D86065
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
