Page MenuHomePhabricator

[TypeSize] Extend UnivariateLinearPolyBase with getWithIncrement/Decrement methods
ClosedPublic

Authored by sdesmalen on Nov 3 2020, 1:38 PM.

Details

Summary

This patch adds getWithIncrement/getWithDecrement methods to
ElementCount and TypeSize to allow:

TypeSize::getFixed(8).getWithIncrement(8)     <=> TypeSize::getFixed(16)
TypeSize::getFixed(16).getWithDecrement(8)    <=> TypeSize::getFixed(8)
TypeSize::getScalable(8).getWithIncrement(8)  <=> TypeSize::getScalable(16)
TypeSize::getScalable(16).getWithDecrement(8) <=> TypeSize::getScalable(8)

This patch implements parts of the POC in D90342.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 3 2020, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2020, 1:38 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript
sdesmalen requested review of this revision.Nov 3 2020, 1:38 PM
dmgreen added a subscriber: dmgreen.Nov 4 2020, 4:23 AM

Can you just overload the + operator? Would that make the calling code more confusing?

I don't know this class very well so feel free to ignore me but I feel like it should be called "getWithIncrement" or something like it. That would be more like Type methods I've seen in the past like getWithNewBitwidth. Like I said if you are going for something else feel free to ignore me though.

Can you just overload the + operator? Would that make the calling code more confusing?

In our SVE/SVE2 sync-up calls we settled on the approach of not using overloaded operators if the operation is not mathematically correct, and instead use named methods. This was the reason for removing operator/ from TypeSize and replacing it with divideCoefficientBy, because it only divides the (compile-time known) minimum value/coefficient and not the full runtime value in (coefficient * vscale) / RHS.

I don't know this class very well so feel free to ignore me but I feel like it should be called "getWithIncrement" or something like it. That would be more like Type methods I've seen in the past like getWithNewBitwidth. Like I said if you are going for something else feel free to ignore me though.

I like getWithIncrement, it seems a better name than add which indeed feels too similar to operator+. I wasn't able to think of a better name for it and therefore went with add, but I like this better. Thanks for the suggestion!

sdesmalen updated this revision to Diff 302832.Nov 4 2020, 6:47 AM
sdesmalen retitled this revision from [TypeSize] Extend UnivariateLinearPolyBase with add/sub methods to [TypeSize] Extend UnivariateLinearPolyBase with getWithIncrement/Decrement methods.
sdesmalen edited the summary of this revision. (Show Details)
  • s/add/getWithIncrement/
  • s/sub/getWithDecrement/
dmgreen added inline comments.Nov 5 2020, 2:51 AM
llvm/include/llvm/Support/TypeSize.h
235

I'm pretty sure my C++ has gotten worse since I started working on compilers.

Is the static_cast here needed? Will that end up just calling the new conversion constructor. Can it just create a LeafTy directly?

sdesmalen added inline comments.Nov 5 2020, 3:22 AM
llvm/include/llvm/Support/TypeSize.h
235

That's a fair question, I played around a bit with the options here. In the end went for a static_cast because it otherwise forces LeafTy to have a constructor of a certain shape (i.e. of whatever is called in this function). Otherwise it depends on a conversion constructor from parent -> child class, which always has the same form.

dmgreen accepted this revision.Nov 5 2020, 8:48 AM

OK fair enough. If the other folks here don't disagree, this LGTM.

This revision is now accepted and ready to land.Nov 5 2020, 8:48 AM
ctetreau accepted this revision.Nov 5 2020, 2:09 PM

looks good to me