Skip to content

Conversation

@anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Oct 3, 2019

Currently, the bias_add expansion is minimal, i.e., it expands the bias to the minimum number of dimensions and then rely on broadcast mechanism. This PR explicitly matches the dimension of bias_add to be exactly same as the other operand. This helps with the later stage in AlterOpLayout.

@icemelon9 @yzhliu @ZihengJiang

Alternative impl - Make AlterOpLayout flexible - #4040

@anijain2305 anijain2305 changed the title [Relay][CanonicalizeOps] Makeing Bias_add shape same as the other operand [Relay][CanonicalizeOps] Make Bias_add shape same as the other operand Oct 3, 2019
@tqchen
Copy link
Member

tqchen commented Oct 3, 2019

This is something that I am not sure about. I do think a better way would be to support general broadcast semantics, because users could reply on it instead of bias add

@anijain2305
Copy link
Contributor Author

Yes, I agree. I don't like this either. This is restrictive only to bias_add. More concerning is that this is sort of leaky, creating dependency between two passes that are far apart. I will close this one.

The alternate one is more generic - #4040 and improves AlterOpLayout braodcast support.
It works for broadcast ops, instead of bias_add. It inserts expand_dims while transforming the layout.

@yzhliu
Copy link
Member

yzhliu commented Oct 3, 2019

I also prefer the one in #4040 and with that it seems we can directly remove this CanonicalizeOps pass.

@anijain2305
Copy link
Contributor Author

Closing this. #4040 covers this.

@anijain2305 anijain2305 closed this Oct 3, 2019
@anijain2305 anijain2305 deleted the canonical branch November 13, 2019 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants