Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbolic shape inference support-2: data propagation-1 #3551

Merged
merged 70 commits into from
Jul 19, 2021

Conversation

jcwchen
Copy link
Member

@jcwchen jcwchen commented Jun 30, 2021

Description

  • Introduce DataPropagationContext, PartialDataPropagationFunction

  • Implement selected operators for PartialDataPropagationFunction as below:

    • Shape
    • Squeeze
    • Unsqueeze
    • Cast
  • Add tests

TODO in another PR:

Add, Concat, Gather, Mul, Reshape, Slice, Size

Motivation and Context
#3506 Second step of symbolic shape inference: partial data propagation

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen requested review from a team as code owners June 30, 2021 22:14
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
}
}

inline void ShapeDataPropagator(DataPropagationContext& ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of this function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after reading the rest of the PR now I understand this is the data propagator for shape op... Please update the function name to make it more clear and add some comments

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
onnx/defs/schema.h Outdated Show resolved Hide resolved
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen changed the title [WIP] Symbolic shape inference support-2: data propagation Symbolic shape inference support-2: data propagation Jul 8, 2021
@jcwchen jcwchen changed the title Symbolic shape inference support-2: data propagation Symbolic shape inference support-2: data propagation A Jul 8, 2021
onnx/defs/shape_inference.h Outdated Show resolved Hide resolved
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
agraph (int32[2,5] x) => (int32[2,5] z)
{
y = Shape(x)
z = Cast(y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Cast" must specify the target of Cast, like "Cast<to = ...>(y)"

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
agraph (int32[2,5] x) => (int32[2,5] w)
{
y = Shape(x)
z = Unsqueeze(y)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding required input "axes" for Unsqueeze

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
z = Unsqueeze(y)
w = Cast(z)
z = Shape(x)
w = Unsqueeze(z, y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make y a constant as y = Constant <...>() to test this out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, that would require a data-propagation for Constant. May be that is the issue (until we allow initializers in the parser).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Since the data propagation of Unsqueeze won't use axes, not sure whether Constant works as expected in this case.

Of course, that would require a data-propagation for Constant. May be that is the issue (until we allow initializers in the parser).

Actually I think we can just skip Constant for data propagation? Constant data can still go into inputDataByName for other op's data propagation function to use, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, my mistake, sorry. Please ignore my comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it is not required for Unsqueeze

Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@jcwchen jcwchen removed the run release CIs Use this label to trigger release tests in CI label Jul 16, 2021
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
@postrational
Copy link
Contributor

Unfortunately code from this change breaks building on older compilers (break occurs on CentOS 7.6)

@jcwchen
Copy link
Member Author

jcwchen commented Aug 24, 2021

Unfortunately code from this change breaks building on older compilers (break occurs on CentOS 7.6)

@postrational Do you have detailed errors that I can look into? Thanks!

@jcwchen jcwchen deleted the jcw/symbolic-2 branch September 1, 2021 16:29
@jcwchen jcwchen mentioned this pull request Sep 1, 2021
@jcwchen jcwchen changed the title Symbolic shape inference support-2: data propagation A Symbolic shape inference support-2: data propagation-1 Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shape inference Issues related to shape inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants