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

[Relay][Pass] CanonicalizeCast #3280

Merged
merged 4 commits into from
Jun 17, 2019
Merged

[Relay][Pass] CanonicalizeCast #3280

merged 4 commits into from
Jun 17, 2019

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Jun 3, 2019

In the case that upcast is multi-refed like:

data(i32) - cast(i8) -cast(i32) - elemwise_op
                                - elemwise_op

if we fuse cast(i32) to the previous op, the sub-function after fusion will have 32-bit output, instead of 8-bit output, which has larger memory access overhead.
In such case, it is better to create a copy of cast(i32):

data(i32) - cast(i8) - cast(i32) - elemwise_op
                     - cast(i32) - elemwise_op

Previously we did special handling in CSE to ignore some common expressions in order to make quantization efficient.
This trick is too fragile for example the recent pr #3233 broke it.
This pass makes it easier as we can transform any expressions into such form that has more efficient fusion result.

cc @tqchen @ZihengJiang @eqy
also feel free to give a better name for this pass

@vinx13 vinx13 force-pushed the fix/quanti_cse branch 2 times, most recently from db277fd to 6179faf Compare June 3, 2019 12:37
@vinx13 vinx13 force-pushed the fix/quanti_cse branch 3 times, most recently from 8b599d6 to 38964db Compare June 5, 2019 02:31
@tqchen
Copy link
Member

tqchen commented Jun 5, 2019

@ZihengJiang @eqy please help to review this PR

python/tvm/relay/ir_pass.py Outdated Show resolved Hide resolved
@jroesch
Copy link
Member

jroesch commented Jun 6, 2019

Can we come up with a better name and put in some docs explaining how this pass works in the text format?

Tianqi wrote this example to me yesterday:

def @main(x: int8) {
    %1 = cast(%x, f32)
    %2 = cast(%x, f32)
    %3 = exp(%1)
    %4 = log(%2)
    (%3, 4)
}

then

def @main(x: int8) {
    %1 = fn (%p1: i8) {
        exp(cast(%p1, f32)
    }
    %3 = %1(%x)
    %2 = fn (%p1: i8) {
        log(cast(%p1, f32)
    }
    %4 = %2(%x)
    (%3, 4)
}

@tqchen
Copy link
Member

tqchen commented Jun 10, 2019

@vinx13 please address review comments. @jroesch can you suggest possible alternatives for naming?

@vinx13 vinx13 force-pushed the fix/quanti_cse branch 3 times, most recently from 74d4e64 to 6c985db Compare June 12, 2019 02:43
@vinx13
Copy link
Member Author

vinx13 commented Jun 13, 2019

I have updated this PR. Here are some of the alternative names in my mind: CanonicalizeCast, CanonicalizeFusion.

@tqchen
Copy link
Member

tqchen commented Jun 13, 2019

CanonicalizeCast seems to be a good name, @jroesch what do you think about it

@jroesch
Copy link
Member

jroesch commented Jun 14, 2019

I think we should use CannoicalizeCast or DuplicateCast or something since this is a cast focused optimization, its not clear to me whether this is always a performance win or not? is that the case?

@tqchen
Copy link
Member

tqchen commented Jun 14, 2019

I think it will always be a perf win.

@vinx13 vinx13 changed the title [Relay][Pass] CanonicalizeExpr [Relay][Pass] CanonicalizeCast Jun 15, 2019
@tqchen tqchen merged commit 04e8162 into apache:master Jun 17, 2019
@tqchen
Copy link
Member

tqchen commented Jun 17, 2019

Thanks @vinx13 @jroesch this PR is now merged

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants