-
Notifications
You must be signed in to change notification settings - Fork 322
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
Use DisposableElementsAttr for ZHigh constant propagation #3013
base: main
Are you sure you want to change the base?
Conversation
… at file writing (onnx#2917)" This reverts commit 33b466e. Signed-off-by: Tung D. Le <[email protected]>
68979b3
to
48cf039
Compare
Signed-off-by: Tung D. Le <[email protected]>
48cf039
to
265ff90
Compare
@tungld Just to understand the high level and without the class names. You are using Soren's approach of applying "logical" operations to the constants so that for example if we have |
Yes, I extend it for ZHigh operations so the same approach is used for both ONNX and ZHigh until lowering to krnl. We can extend it to cover krnl operations but it needs more work and I didn't do it in this PR. |
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
struct ConstantStickPattern : public OpRewritePattern<ZHighStickOp> { | ||
ConstantStickPattern(MLIRContext *context) : OpRewritePattern(context) {} | ||
LogicalResult matchAndRewrite( | ||
ZHighStickOp stickOp, PatternRewriter &rewriter) const override { | ||
Value input = stickOp.getIn(); | ||
Value output = stickOp.getOut(); | ||
StringAttr layout = stickOp.getLayoutAttr(); | ||
|
||
// Match | ||
if (!isDenseONNXConstant(input)) { | ||
return failure(); | ||
} | ||
|
||
// Rewrite | ||
Value stickifiedVal = | ||
createConstantForStick(rewriter, output, input, layout); | ||
replaceOpAndGC(rewriter, stickOp, stickifiedVal); | ||
return success(); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, you replaced tablegen with cpp. Is this because replaceOpAndGC()
is difficult to use in tablegen format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't know how to do that with tablegen, let me know if you know how to do it. Thanks!
Can you post here for ref the improvements you got, just for future reference purpose. Does not need to be super detailed. Thanks |
@jenkins-droid test this please |
store-constants-to-file
is off.ONNXConstantOp
) toZHighStickifiedConstantOp
, so that the zhigh constant prop can benefit from the memory management forDisposableElementsAttr
: In particular, it does:DisposableElementsAttr
ZHighStikifiedConstantOp
to acceptDisposableElementsAttr
. Its parser and printer are changed to read/write from/toDenseElementsAttr
for lit tests.ZHighConstantPropagationPass
so that it reads data directly fromDisposableElementsAttr
instead ofDenseElementsAttr
.ZHighDisposableGabageCollector
andZHighScrubDisposablePass
to manage buffers used byZHighStickifiedConstantOp
Quick experiment: the peak compile memory consumptions of #2917 and this PR when compiling the gpt2-large model for NNPA (744M parameters, the constant file's size is 3.2GB) are quite similar, both are about 9GB.
This patch contains the reverting code so it's no easy to follow. To ease the review, I merge all new changes (not the reverting code) into a single commit: 265ff90. Please look at this commit for review.