Introduce "valid dimension", used as needed when calculating operand shapes

[inexorabletash]

Certain operands like reshape2d() calculate dimensions in their output shape with expressions like shape[i] * scales[i] which could be larger than can be represented as a dimension in MLOperandDescriptor.

Introduce a "valid dimension" term, use it when calculating output dimensions in the following ops, and throw if needed.

• constant() (sequential fill)
• concat()
• conv2d()
• convTranspose2d()
• pad()
• pooling operators
• resample2d()
• reshape()

Fixes #610

---

Preview | Diff

[inexorabletash]

Up for discussion. A few more thoughts here:

• We'll want to roll in any change needed by Allow 0 size dimensions (dimensions containing a 0 in the list of sizes, not a rank of 0 which is valid) #391
• It might be cleaner to inline the steps, rather than having "...then return failure." and "If that returns failure..." I kept the change minimal for now. Discuss!
• If we anticipate this occurring elsewhere in the spec then we could introduce a definition like:

A valid dimension is an integer greater than zero in the range of unsigned long. Implementations may impose a lower upper limit.

(dropping "greater than zero" depending on #391)

... and then change check dimensions to say:

If any element of descriptor.dimensions is not a valid dimension, then return false.

Thoughts?

[inexorabletash]

If we introduce a "valid dimension" definition, we probably want to use it in these other places that calculate a dimension:

• constant
• conv2d
• convTranspose2d
• pad
• pooling operations
• reshape

... which are the obvious places where a dimension is assigned from a calculation. Probably more places.

[huningxin]

Probably more places.

• concat
• gru and gruCell, their steps compare a dimension with 3 * hiddenSize that might be a invalid unsigned long.
• lstm and lstmCell, their steps compare a dimension with 4 * hiddenSize that might be a invalid unsigned long.

[inexorabletash]

• concat

Ah yes, it's a sum of other dimensions, so could overflow.

• gru and gruCell, their steps compare a dimension with 3 * hiddenSize that might be a invalid unsigned long.
• lstm and lstmCell, their steps compare a dimension with 4 * hiddenSize that might be a invalid unsigned long.

I think those are cases where the intermediate value could overflow if the math is done in uint32 space, but the final values used for dimensions are always valid uint32, since they come directly from other MLOperands. So those are cases where #636 applies.

I'm gonna update this PR to introduce the "valid dimension" term (done in e737dcb). Should I go ahead and expand it beyond resample2d() or do that in another PR?

[huningxin]

LGTM!

[huningxin]

Thanks @inexorabletash !

Should I go ahead and expand it beyond resample2d() or do that in another PR?

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

[inexorabletash]

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

Done in 4f30583 - I'll update the PR name / description

[huningxin]

LGTM with a nit

[inexorabletash]

@fdwr - can you please review and merge if it looks good to you? Thanks!

[fdwr]

Tiny wording thought (do you prefer one over the other? I'll go with whatever you like.), else LGTM. Thanks, as I like centralizing this dimension validity rather than it being scattered around.

[inexorabletash]

Suggested wording change was great @fdwr - it felt awkward as I was writing it.