[TIR][Bugfix] Improved massive build times caused by tir.floormod and tir.floordiv. Fixed Topi testcase. #5666

dprankratz · 2020-05-24T16:51:24Z

Build times

I experienced hangs when attempting to build a compute statement that depended on deep TIR expressions involving the tir.floormod and tir.floordiv operaors. The build times can be reproduced with the script here. This is due to the very complicated TIR expression generated when FloorMod or FloorDiv are lowered.

I was able to improve upon this by changing how these operators were lowered to use the tir.floor intrinsic which also has the benefit of matching the definitions of the operators.

Topi testcase

There are existing tests for tir.floormod and tir.floordiv through the topi test_topi_broadcast.py file. However, I noticed that this files used np.fmod and np.floor_divide as points of reference which are not equivalent to floormod and floordiv.

I fixed the testcase to use a correct version of floormod and floordiv.

ghost · 2020-05-24T22:19:56Z

Reviewed.

tqchen · 2020-05-24T23:13:56Z

Thanks @dpankratz , in this particular case, maybe we could try to go and fix the integer lowering path. How about wrap the temp value creation with a LetNode?

dprankratz · 2020-05-25T01:54:30Z

@tqchen I'm not sure how to gracefully add a LetNode since I'm running into trouble creating a variable with a unique name. For example, using a simple counter to create variables of the form temp0, temp1 will run into problems when multiple expressions exist concurrently and the names conflict causing SSA form to be broken.

I've tried searching for other places where creating variables with unique names is handled and haven't seen anything which makes me uneasy. Any suggestions?

tqchen · 2020-05-25T02:30:53Z

the name hint of the variable is not required to be unique, as the address of the variable is used to uniquely identify the var

tqchen · 2020-06-11T20:02:34Z

cc @dprankratz can we followup

dprankratz · 2020-06-14T19:03:20Z

@tqchen I did some more digging into using let nodes. Consider the following expression:

floormod(floormod(a,b),floormod(c,d))

floormod(a,b) becomes let rmod1 = (a % b) in (expression with multiple rmod1)
floormod(c,d) becomes let rmod2 = (c % d) in (expression with multiple rmod2)

Then composing those with another floormod ends up creating multiple copies of the let statements using rmod1 and rmod2.

floormod(floormod(a,b),floormod(c,d)) becomes

let rmod3 = (let rmod1 = .... in ....) % (let rmod2 = ... in ....) in
   (expression with multiple rmod3)

The net effect is that multiple rmod3 copies are materialized which contain LetNodes for rmod1 and rmod2 which have the same address. This ends up failing the VarUseDefAnalysis pass since it believes the redefinitions of rmod1 and rmod2 break SSA.

Is there an easy solution to this problem?

tqchen · 2020-06-28T16:32:23Z

@dpankratz sorry for the delayed reply. #5949 relaxes the constraint of the Let to allow a single var to be binded multiple times as long as the binding values are the same. Can you please try again?

tqchen · 2020-06-28T23:13:31Z

src/tir/transforms/lower_intrin.cc

+        */
+
+        // floor(a / b)
+        auto fdtype = DataType::Float(dtype.bits() * 2, dtype.lanes());


Let us also use the rules in the else branch instead (in case the target does not support floating pts but support integer arithmetics)

I changed the logic in the most recent commit. Thanks for the suggestion!

tqchen · 2020-06-28T23:13:54Z

src/tir/transforms/lower_intrin.cc

+
+        // a - floor(a / b) * b
+        auto fdtype = DataType::Float(dtype.bits() * 2, dtype.lanes());
+        auto div = tir::Div(tir::Cast(fdtype, op->a), tir::Cast(fdtype, op->b));


Let us also use the rules in the else branch instead (in case the target does not support floating pts but support integer arithmetics)

…n np floor_div and fmod.

…orDiv using let nodes

…des.

…that don't support it.

tqchen · 2020-07-28T16:06:22Z

Sorry for the long delay, Thanks @dpankratz ! This PR is now merged

… tir.floordiv. Fixed Topi testcase. (apache#5666) * Improved uncommon case of floormod and floordiv. Removed dependence on np floor_div and fmod. * Fixed clang-format complaints * Streamlined floormod and floordiv lowering logic * Improved build times by expressing int64 case of tir FloorMod and FloorDiv using let nodes * Updated use-def analysis and llvm codegen to support duplicated letnodes. * Corrected misuse of var_map_ in llvm codegen * Updated backends that support LetNode * Changed floormod and div lowering logic to avoid using FP on systems that don't support it. * Fixed formatting Co-authored-by: pankratz <[email protected]>

tqchen added the status: need review label May 24, 2020

dprankratz force-pushed the master branch from 2e58173 to 1f2b1c6 Compare June 28, 2020 21:20

tqchen requested changes Jun 28, 2020

View reviewed changes

dpankratz added 8 commits July 1, 2020 10:04

Improved uncommon case of floormod and floordiv. Removed dependence o…

6b5b198

…n np floor_div and fmod.

Fixed clang-format complaints

f1ab807

Streamlined floormod and floordiv lowering logic

15a7f1e

Improved build times by expressing int64 case of tir FloorMod and Flo…

2902f7d

…orDiv using let nodes

Updated use-def analysis and llvm codegen to support duplicated letno…

67cc96c

…des.

Corrected misuse of var_map_ in llvm codegen

f0dd32e

Updated backends that support LetNode

2400db8

Changed floormod and div lowering logic to avoid using FP on systems …

4593a9a

…that don't support it.

dprankratz force-pushed the master branch from 9263ab2 to 4593a9a Compare July 1, 2020 16:27

Fixed formatting

54c739a

tqchen approved these changes Jul 28, 2020

View reviewed changes

tqchen merged commit a02d377 into apache:master Jul 28, 2020

tqchen added status: accepted and removed status: need review labels Jul 28, 2020

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIR][Bugfix] Improved massive build times caused by tir.floormod and tir.floordiv. Fixed Topi testcase. #5666

[TIR][Bugfix] Improved massive build times caused by tir.floormod and tir.floordiv. Fixed Topi testcase. #5666

dprankratz commented May 24, 2020

ghost commented May 24, 2020

tqchen commented May 24, 2020

dprankratz commented May 25, 2020 •

edited

Loading

tqchen commented May 25, 2020

tqchen commented Jun 11, 2020

dprankratz commented Jun 14, 2020

tqchen commented Jun 28, 2020

tqchen Jun 28, 2020 •

edited

Loading

dprankratz Jul 1, 2020

tqchen Jun 28, 2020 •

edited

Loading

tqchen commented Jul 28, 2020

[TIR][Bugfix] Improved massive build times caused by tir.floormod and tir.floordiv. Fixed Topi testcase. #5666

[TIR][Bugfix] Improved massive build times caused by tir.floormod and tir.floordiv. Fixed Topi testcase. #5666

Conversation

dprankratz commented May 24, 2020

Build times

Topi testcase

ghost commented May 24, 2020

tqchen commented May 24, 2020

dprankratz commented May 25, 2020 • edited Loading

tqchen commented May 25, 2020

tqchen commented Jun 11, 2020

dprankratz commented Jun 14, 2020

tqchen commented Jun 28, 2020

tqchen Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

dprankratz Jul 1, 2020

Choose a reason for hiding this comment

tqchen Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

tqchen commented Jul 28, 2020

dprankratz commented May 25, 2020 •

edited

Loading

tqchen Jun 28, 2020 •

edited

Loading

tqchen Jun 28, 2020 •

edited

Loading