Closes 5113, fixing behavior of floor division operator (//) #5131
Conversation
drculhane
force-pushed
the
Closes-5113
branch
from
29b003f to
72ccf64
Compare
1RyanK
left a comment
1RyanK
left a comment
There was a problem hiding this comment.
I think the PR is mostly correct, just a bit of a concern here.
src/BinOp.chpl
Outdated
| res += divisor; | ||
| } | ||
| return res; | ||
| inline proc floorDivisionHelper(dividend : ?tn, divisor : ?td) : floorDivisionType(tn,td) { |
There was a problem hiding this comment.
I feel like this is slightly heavier machinery than we need on int, uint, bool. I'm slightly concerned it may impact performance. Similar with modHelper I think
| return -1:real; | ||
| proc floorDivision (dividend: [?d] ?tn, divisor: [d] ?td, type etype) : [d] etype throws { | ||
| var quotient = makeDistArray(d, etype); | ||
| coforall loc in Locales do on loc { |
There was a problem hiding this comment.
I don't think it's quite necessary to make the parallelism so explicit here. Read the first paragraph here, and then the noted equivalency here.
I guess this should more or less be the same thing but I think if we're worried about parallel zippered iteration in terms of performance, I'm pretty sure you can just do a forall over the domain and let Chapel do the heavy lifting on the distributing (technically as long as all arrays are distributed the same, but since we don't really give the user control over the distribution, it should be fine).
3 participants
Closes #5113
Update -- after a chat with Ryan, I realized that it wasn't really enough to correct the behavior of // without also distributing it. So this version does that, with a new function called floorDivision, which calls a slightly changed floorDivisionHelper.
There are three versions of floorDivision, for the vv, vs and sv cases.
All of the calls to floorDivisionHelper in BinOp, which were zippered foralls, now call floorDivision.
I also changed the calls to floorDivisionHelper in TimeClassMsg.chpl and OperatorMsg.chpl.
Also at Ryan's suggestion, I removed "inline" from the declaration of floorDivisionHelper. I haven't yet addressed ModHelper. That will be in the PR for the behavior of the % operator.
Previously -- I modified floorDivisionHelper to handle more data types, and invoked it in places where chapel had been performing a simple (and incorrect) a/b division.