This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Replace / operator in TypeSize/ElementCount with divideCoefficientBy
ClosedPublic

Authored by david-arm on Sep 15 2020, 8:24 AM.

Details

Summary

After some recent upstream discussion we decided that it was best
to avoid having the / operator for both ElementCount and TypeSize,
since this could give the impression that these classes can be used
in the same way as basic integer integer types. However, division
for scalable types is a bit odd because we are only dividing the
minimum quantity by a value, as opposed to something like:

(MinSize * Vscale) / SomeValue

This is why when performing division it's important the caller
first establishes whether the operation makes sense, perhaps by
calling isKnownMultipleOf() prior to division. The caller must now
explictly call divideCoefficientBy() on the class to perform the operation.

Diff Detail

Event Timeline

david-arm created this revision.Sep 15 2020, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 8:24 AM
david-arm requested review of this revision.Sep 15 2020, 8:24 AM
sdesmalen added inline comments.Sep 17 2020, 6:11 AM
llvm/include/llvm/Support/TypeSize.h
66

As we discussed in the SVE sync-up call, mul isn't strictly needed but rather added for consistency.
That raises the question whether operator- and operator+ also need to have a sub and add functions, as those are not "mathematically correct" either given they only support adding either 2 scalable, or 2 fixed-sized sizes, and they assert otherwise.

252

Given the particular reasons for adding these in favour of the overloaded operators, these functions need a clear description that explains why we've done so, and why the results may not be entirely arithmatically correct when uses of this function assume vscale to be some specific value at compile-time.

305

Please add a description for this method.

ctetreau added inline comments.Sep 17 2020, 7:43 AM
llvm/include/llvm/Support/TypeSize.h
66

I think given the outcome of the call today, operator* can probably stay.

If we're going to document the header that "if it behaves the same as in real math, then it's an operator. If not, it's a named function", then this multiply is fine.

69–78

I would suggest renaming this to something more self-documenting. Perhaps coefficientDiv? This has the added benefit of making it seem less strange that it's a named divide rather than operator/

240–252

Same comment as ElementCount

david-arm updated this revision to Diff 293135.Sep 21 2020, 4:15 AM
david-arm retitled this revision from [SVE] Replace *, / operators in TypeSize/ElementCount with mul, div to [SVE] Replace / operator in TypeSize/ElementCount with div.
david-arm edited the summary of this revision. (Show Details)
  • Reverted the operator* change.
  • Added comments to the new functions.
david-arm marked 6 inline comments as done.Sep 21 2020, 4:16 AM
david-arm retitled this revision from [SVE] Replace / operator in TypeSize/ElementCount with div to [SVE] Replace / operator in TypeSize/ElementCount with coefficientDiv.
sdesmalen added inline comments.Sep 22 2020, 9:27 AM
llvm/include/llvm/Support/TypeSize.h
69–78

Agree that the use of coefficient is a good addition. Perhaps this is getting into bike-shedding territory, but then I'd probably prefer the more verbose divideCoefficientBy as this reads a bit more intuitive to me (and follows a natural way of phrasing similar to isKnownMultipleOf). Any thoughts?

I'm not a great fan of the coefficientDiv name but once in I doubt I'll give it a second thought. That said, because by far the most common usage is coefficientDiv(2), whose intent is clearly to knowingly split something into two equal parts I'm wondering if adding a halve() utility function will keep the main usage short and meaningful and then there's even less reason to care about the name of coefficientDiv.

llvm/include/llvm/Support/TypeSize.h
86–89

I don't think it's a good idea to promote incompatible usages of this function. Let's keep the messaging simple with only a true result being something that is meaningful, which is inline with the other isKnown methods.

306–309

As above.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19524–19525

Should this use isKnownMultipleOf to match the coefficientDiv you've introduced?

llvm/lib/CodeGen/TargetLoweringBase.cpp
835–836

SVT.getHalfNumVectorElementsVT(Context) ?

llvm/unittests/CodeGen/ScalableVectorMVTsTest.cpp
64

Comment needs updating, I'd just drop the overloaded '*' and '/' part.

I think that introducing a halve() function makes sense and I'll replace div(2) with halve(), unless someone strongly objects? I don't have strong feelings about the name to be honest, I'm personally happy enough to stick with coefficientDiv. @ctetreau any thoughts on Sander's suggestion of divideCoefficientBy?

david-arm updated this revision to Diff 294064.Sep 24 2020, 8:01 AM
david-arm edited the summary of this revision. (Show Details)
  • Introduced a new halve() function to replace the common case of coefficientDiv(2).
  • Updated the comments about isKnownMultipleOf.
david-arm marked 5 inline comments as done.Sep 24 2020, 8:02 AM
ctetreau added a comment.EditedSep 24 2020, 9:07 AM

... thoughts on Sander's suggestion of divideCoefficientBy?

I think the important thing is that the function has "coefficient" in the name. I prefer my name because it's shorter, but if more people prefer Sander's suggestion that's fine.

My general philosophy is "the name should imply what the function does" and not "the name should read like an english sentence". We're not COBOL programmers after all :)

I'm not in love with the halve function. I count 8 places where it's used in real code in this patch, which really isn't that much. I think having it in there will establish a precedent and encourage people to add a million "common case" helpers (why not double? quarter/quadruple? next/prev power of 2?) for constructing different sized ElementCounts.

Really, EC.halve() isn't that much less verbose than EC.coefficientDiv(2), and I suspect it will usually appear on its own line as an assignment since VectorType already has getHalfElementsVectorType.

I think the precedent has already been set as VectorType and ValueType already deem doubling and halving to be special enough to have explicit operations, albeit they save more effort that what's being saved here. Personally I just think

ElementCount.halve()

is more meaningful/readable than

assert(ElementCount.isKnownMultiple(2));
ElementCount.coefficientDiv(2);

Whereas ElementCount * 2 is already meaningful enough.

I just wanted to voice my concerns for halve(). If nobody else has an opinion on the subject, then I'll not hold the review up over it.

I'm fine with the substance of this patch. Once we come to an agreement about what color the bikeshed should be, I think it's good to go.

@david-arm The main priority is to remove the operator overloads so if universally using divideCoefficientBy pleases the most people then let's just do that.

llvm/unittests/CodeGen/ScalableVectorMVTsTest.cpp
66–67

If halve is kept then it makes sense to add an explicit test for it here.

66–67

Oops, please ignore what looks like a stale review comment :(

david-arm updated this revision to Diff 294351.Sep 25 2020, 9:42 AM
david-arm retitled this revision from [SVE] Replace / operator in TypeSize/ElementCount with coefficientDiv to [SVE] Replace / operator in TypeSize/ElementCount with divideCoefficientBy.
david-arm edited the summary of this revision. (Show Details)

I'm happy colour of the bikeshed. Please re-run clang-format on it though.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2640

clang-format?

paulwalker-arm accepted this revision.Sep 25 2020, 11:06 AM

Formatting issues aside this LGTM.

This revision is now accepted and ready to land.Sep 25 2020, 11:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 12:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript