typically? when are they not?

Seems odd that these can be invalid. It makes it hard to know statically when they are not used.

Alternatively, because in every place we accept a shapesymbol , it might be unknown. We can explictly reserve one value of the integer to mean "unknown shape", which is not equal to any other unknown shape. This would prevent having to wrap all ShapeSymbols in optionals. In other words Shape symbols would represent something like:

• ? - completely unknown, encoding 0
• 3 - literally size 3, encoding 3
• a - the same size as other a, encoding -1

I don't think these should be public. This is suppose to be an abstract class representing a symbol, not a containing for our particular encoding.

We are trying to keep shapes separate from integers, but this sort of undoes that by allowing all shapes to be used as integers anywhere.

I will get rid of this in the next patch, since the new algorithm doesn't use isCompatibleWithInCurrentExecutionContext

If we already use negative numbers to represent abstract shapes, then why is this necesary?

I find this confusing, both from a template perspective and from an actual behavior perspective. We don't want to pun integers and shapes but this is extending it through the API.

• stride and contiguity information should probably go in its own object not flattened out in the tensor.
• stride_indices_ ,and contiguity_, and strides_ need documentation. I think you can take a lot of the documentation from the quip document that proposes this interface.

stride and contiguity information should probably go in its own object not flattened out in the tensor.

Can we defer this piece until a bit later? I understand we want a StrideDimension object that has an index and contiguity but since there isn't any code that actually uses this information (e.g. contiguity and stride_indices) yet.

Well, there at least should be tests that this information is getting tracked correctly. I found what seem like several bugs tracking this information. If it is unused, then it shouldn't be going into the PR.

merge_sizes is unused, its a bit confusing to have it. If you want to not merge sizes, you can just blank out the sizes after or before the merge.

it's used in the next PR since to avoid running the default logic for VaryingShape<ShapeSymbol> since the merging logic will be in profiling_record.cpp

That doesn't sound like a good separation of concerns, but it is difficult to suggest something better because its use isn't in this PR. Maybe remove it from here and put in the PR that actually uses it. I am skeptical that this is the best way to represent something, because this boolean flag essentially makes merge not perform a merge, which is an abstraction leak issue.

Did this move, or does the just remove the hashing test? Seems like an important test.

Forgot to port it. Also changed the tests that use TensorType::create in a way we actually use it.

Implementation details of TensorType appear to have leaked here. I think it would look clearer of stride information was represented as its own object.

I will get rid of this in the next patch, since the new algorithm doesn't use isCompatibleWithInCurrentExecutionContext

I wanted to defer an API clean-up until we have the entire E2E pipeline working for dynamic shapes, but it's indeed pretty confusing in its current state, so I cleaned it up.

auto s = Shape::fromConstantSize(3);

added fromStaticSize

auto n = ShapeScope::fresh(); // new shape in the scope (i.e. the function being profiled)

Unfortunately, this needs to be a bit more complex than this since ShapeScope needs to persist across multiple profiles of the same function. For the first iteration, I just added newSymbol which generates a fresh symbol unique globally across all the functions.

std::optional concrete_value = s.static_size();

Added static_size returning int and is_static

added a comment

it's used in the next PR since to avoid running the default logic for VaryingShape<ShapeSymbol> since the merging logic will be in profiling_record.cpp

stride and contiguity information should probably go in its own object not flattened out in the tensor.

Can we defer this piece until a bit later? I understand we want a StrideDimension object that has an index and contiguity but since there isn't any code that actually uses this information (e.g. contiguity and stride_indices) yet.

Forgot to port it. Also changed the tests that use TensorType::create in a way we actually use it.

Shouldn't contiguity be determined after sorting? If we know that we are handling transposed data, for kernel generation, what we would care about is the contiguity information in ordered stride.

correct, contiguity information is suppose to be from smallest to largest. So this probably needs to be done in verse order: first compute stride_indices, then contiguity form them.

To make this problem even more convoluted, currently we sort dimensions by their stride and break tie on their semantic index (https://github.com/pytorch/pytorch/pull/36101/files/4366d3d892bd89811c11483474e0917f612c3f1d#diff-1eaac5e95929476c6522cd50bbb2432eR781)

However, it makes the contiguity information ambiguous.
If we have an unsqueezed tensor with size [4, 1, 8] and a stride [8, 1, 1], current implementation would compare contiguity between dimension [0/1] & [1/2] where it won't correctly identify that the meaningful dimension [0/2] are actually contiguous.

Size-1 dimension makes contiguity tricky to handle for two reasons: 1. stride could be wild; 2. it breaks the cumulative law of contiguity;
I think we should special case size-1 dimension to make our life easier:

1. We still need to preserve size-1 dimension when we sort the dimensions per their stride. (This is to ensure our memory_format stuff works properly)
2. size-1 tensor should transparently pass contiguity check and instead assess contiguity information for closest neighboring dimensions with size != 1;

I'm in favor of Zach's idea of having a separate class to properly encapsulate this logic.

should probably be explicit to prevent confusion.

Fresh symbols should have negative values, right? Otherwise the symbol gets a fixed size?

what does is_static mean? this could use a comment.

What does 'fixed' mean? (I can guess but developers new to the codebase could use more specific wording).

The problem with representing stride_indices and continuity as separate VaryingShape elements is that it offers more ways in which the information is present/absent than what is actually expected. For instance, is there any case where we set some stride_index values but not the contiguity values? The current data structures can represent that, but it is not clear that was the intent vs. just a side effect of exposing the arrays as VaryingShape objects. In its current form any code that needs to use this information has to be much more verbose to handle a bunch of cases that don't actually happen. I still think it would be much better to create an abstract object that represents the striding information and documentation about what it means.

this looks like it is missing correct handling of stride_indices_ and contiguity_. This is another reasons to implement strides a a self-contained object: a bunch of splatted parameters make it harder to keep objects up to date.

This seems like it doesn't correct set the rest of the stride inforation

That doesn't sound like a good separation of concerns, but it is difficult to suggest something better because its use isn't in this PR. Maybe remove it from here and put in the PR that actually uses it. I am skeptical that this is the best way to represent something, because this boolean flag essentially makes merge not perform a merge, which is an abstraction leak issue.

Well, there at least should be tests that this information is getting tracked correctly. I found what seem like several bugs tracking this information. If it is unused, then it shouldn't be going into the PR.

This is a lot of extra code to avoid creating a SymbolicShape object from an integer. I would prefer explicitly doing that over this complicated template logic.

correct, contiguity information is suppose to be from smallest to largest. So this probably needs to be done in verse order: first compute stride_indices, then contiguity form them.

can you add a comment about how this encodes the above information?

Why not just use Stride() to represent an empty stride? Seems confusing to have to de-normalize and renormalize things.

What does 'fixed' mean? (I can guess but developers new to the codebase could use more specific wording).

First time this is called, it will return -1 which is the same as ShapeSymbol(). Since -1 is being used to encode a non-present shape, this seems wrong.

num_symbols starts with 1 (i.e. there's always one unknown/undefined symbol). newSymbol increments first then returns, so the first new symbol should have the index of -2. I also ran a quick test to confirm and I'm indeed seeing -2

nit: stride_properties, it isn't a great idea to abbreviate in the public API of a class.

Both Stride, and ShapeSymbol now have their own internal ways of representing not present values. So VaryingShape is going to get confusing because of the possibilities involved. It would be good to try to simply the API surface here. For instance does sizes() need to still return a list of optional ints? Or do we just need concrete sizes, and sizes_properaties() (i.e. the symbolic shape-returning version).

I initially liked the consistency of having c10::optional to denote the unknowness but it's getting a bit clunky and awkward especially with Strides.
We could probably switch to the following API:

c10::optional<std::vector<ShapeSymbol>> symbolic_sizes();
c10::optional<std::vector<int64_t> concrete_sizes();
//strides
c10::optional<std::vector<Stride>> stride_properties();
// and we should be removing `concrete_strides()`/`strides()`

stride_indices, contiguity are unused here. Doesn't seem to be implemented correctly but the code makes it look like it is.

oh :-( yes, that's definitely a bug. we should use computeStrideProps

oh wait, no it's correct. that particular constructor we call is a convenience c-tor. If VaryingShapes contain concrete sizes we will compute the stride properties, otherwise we will just compute the rank information.

Shouldn't we also mention implicit broadcasting here where we have dimension with stride == 0?

yup, i also want to switch contiguous to YES, NO, BROADCASTED .

Quick question: since stride_ is an c10::optional<size_t> basically means it's only carrying static stride when it's applicable. Meanwhile, contiguious_ information is only really useful when we have symbolic shapes -> non-static stride_. I see an opportunity to merge the two together, but I don't know if I missed out anything.

And certainly not a priority in this PR.

@jjsjann123 we will be deprecating and removing strides_. It's basically there to support the legacy executor.

NITPICK: we have value_ as int64_t, which is not enough range for pytorch size_t. I guess realistically we shouldn't run into issues but maybe we can put a comment here.

sure i'll do it in the next PR since it's very painful to land even this one.