[PATCH] D86065: [SVE] Make ElementCount members private

rsandifo-arm added a comment. In D86065#2241146 , @david-arm wrote: > Hi @ctetreau, ok for now I'm going to completely remove the operators and > revert the code using those operators to how it was before. I'm not sure what > you mean about the

[PATCH] D86065: [SVE] Make ElementCount members private

This revision was automatically updated to reflect the committed changes. Closed by commit rGf4257c5832aa: [SVE] Make ElementCount members private (authored by david-arm). Changed prior to commit: https://reviews.llvm.org/D86065?vs=288370=288594#toc Repository: rG LLVM Github Monorepo

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added a comment. To be more clear, I'm happy to defer the divide conversation for if/when we run into issues so my previous acceptance still stands. It'll be good to get the intent of the patch in (i.e. stoping access to internal class members) asap, plus any follow up work

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added a comment. I'm retracting my operator% request. After thinking about it and speaking with Dave I just cannot see how allowing a total divide is safe for scalable vectors. If you are relying on a truncating divide then special handling is require anyway, which is likely to

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added a comment. Can't say I agree since people are already writing the ugly code, because the result typically demands different handling or they're asserting the divide doesn't truncate in the first place. That said I'm happy for there to be no assert as long as operator% is

[PATCH] D86065: [SVE] Make ElementCount members private

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 /=(unsigned RHS) { +Min /=

[PATCH] D86065: [SVE] Make ElementCount members private

ctetreau added a comment. In D86065#2241146 , @david-arm wrote: > Hi @ctetreau, ok for now I'm going to completely remove the operators and > revert the code using those operators to how it was before. ... This is probably for the best. In

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm accepted this revision. paulwalker-arm added a comment. This revision is now accepted and ready to land. There's probably a few .Min to .getKnownMinValue() conversions where the .Min could be dropped (calls to Builder.CreateVectorSplat for example) but they can be tidied up as

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm updated this revision to Diff 288370. david-arm added a comment. - Removed isPowerOf2() function since this is potentially misleading - it's only the known minimum value that we're checking. - Renamed isEven to isKnownEven to try and make it clear that returning true indicates we know

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added a comment. I cannot say whether such questions make sense without a deeper investigation, but I can say for certain that EC.isPowerOf2 is a question we cannot answer at compile time. Given this is a mechanical change I would just remove the member function and leave the

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:108 + + bool isPowerOf2() const { return isPowerOf2_32(Min); } }; paulwalker-arm wrote: > I don't believe this is safe. For example we know SVE supported vector > lengths only

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:108 + + bool isPowerOf2() const { return isPowerOf2_32(Min); } }; I don't believe this is safe. For example we know SVE supported vector lengths only have to be a multiple

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm updated this revision to Diff 288260. david-arm edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86065/new/ https://reviews.llvm.org/D86065 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/lib/CodeGen/CodeGenTypes.cpp

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm added a comment. Hi @ctetreau, ok for now I'm going to completely remove the operators and revert the code using those operators to how it was before. I'm not sure what you mean about the predicate functions so I've left those for now, since they aren't needed for this patch. The

[PATCH] D86065: [SVE] Make ElementCount members private

ctetreau added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:56 + friend bool operator>(const ElementCount , const ElementCount ) { +assert(LHS.Scalable == RHS.Scalable && fpetrogalli wrote: > paulwalker-arm wrote: > > david-arm wrote:

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm updated this revision to Diff 287672. david-arm added a comment. Herald added a subscriber: rogfer01. - Changed comparison function from gt to ogt and added a olt (less than) comparison function too. - Instead of adding the ">>=" operator I've added "/=" instead as I think this is

[PATCH] D86065: [SVE] Make ElementCount members private

fpetrogalli added a comment. In D86065#2235434 , @david-arm wrote: > Hi @fpetrogalli, if you don't mind I think I'll stick with Paul's idea for > ogt because this matches the IR neatly, i.e. "fcmp ogt". Also, for me > personally it's much simpler and

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm added a comment. Hi @fpetrogalli, if you don't mind I think I'll stick with Paul's idea for ogt because this matches the IR neatly, i.e. "fcmp ogt". Also, for me personally it's much simpler and more intuitive. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86065/new/

[PATCH] D86065: [SVE] Make ElementCount members private

fpetrogalli added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:56 + friend bool operator>(const ElementCount , const ElementCount ) { +assert(LHS.Scalable == RHS.Scalable && paulwalker-arm wrote: > david-arm wrote: > > ctetreau wrote:

[PATCH] D86065: [SVE] Make ElementCount members private

paulwalker-arm added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:56 + friend bool operator>(const ElementCount , const ElementCount ) { +assert(LHS.Scalable == RHS.Scalable && david-arm wrote: > ctetreau wrote: > > fpetrogalli wrote:

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm updated this revision to Diff 287003. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86065/new/ https://reviews.llvm.org/D86065 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/lib/CodeGen/CodeGenTypes.cpp llvm/include/llvm/Analysis/VectorUtils.h

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm added a comment. Hi @ctetreau, I agree with @efriedma that keeping the two classes distinct for now seems best. The reason is I spent quite a lot of time trying to unify these classes already and I hit a stumbling block - TypeSize has the ugly uint64_t() cast operator, which makes

[PATCH] D86065: [SVE] Make ElementCount members private

efriedma added a comment. > Perhaps now would be a good time to combine TypeSize and ElementCount into a > single Polynomial type? We don't have to implement the whole abstraction of > c*x^n (since we currently don't use the exponent, and don't distinguish > between X's) but if it's ever

[PATCH] D86065: [SVE] Make ElementCount members private

ctetreau added a comment. Perhaps now would be a good time to combine TypeSize and ElementCount into a single Polynomial type? We don't have to implement the whole abstraction of `c*x^n` (since we currently don't use the exponent, and don't distinguish between X's) but if it's ever needed in

[PATCH] D86065: [SVE] Make ElementCount members private

fpetrogalli added inline comments. Comment at: llvm/include/llvm/Support/TypeSize.h:56 + friend bool operator>(const ElementCount , const ElementCount ) { +assert(LHS.Scalable == RHS.Scalable && I think that @ctetreau is right on

[PATCH] D86065: [SVE] Make ElementCount members private

david-arm created this revision. david-arm added reviewers: sdesmalen, ctetreau, efriedma, fpetrogalli, kmclaughlin, c-rhodes. Herald added subscribers: llvm-commits, cfe-commits, psnobl, hiraditya, tschuett. Herald added projects: clang, LLVM. david-arm requested review of this revision. This