-
Notifications
You must be signed in to change notification settings - Fork 12
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
While simplify opt #248
While simplify opt #248
Conversation
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.
Yeah I think you should update this as a special case of constants.
Also clearly we want a follow up pass here [which may be https://github.com//pull/173] to remove unused iter args (or those which only copmute things for themselves. e.g. if you have the equivalent of
%res = while iter = %0 {
} {
nextiter = iter * 10
yield nextiter
}
and res is unused. We don't need the iter arrgument, nor nextiter [since it is only used to compute iter]
@GleasonK I remember somewhere seeing that the spec for whileop [perhaps in practice] required all variables used within the whileop region to be passed in as iterargs. However, I don't see that in the spec: https://github.com/openxla/stablehlo/blob/main/docs/spec.md#while Can you remind me what the requirements are ? |
Hm, I'll look more closely next week but that's an HLO requirement, not sure if it's a stablehlo req, we have an MLIR pass to fix up any while ops that violate the HLO requirement |
Interesting, if you have a link to that that would be good to look at. There are some potential questions / perf optimizations there (for example one could choose to either pass both X and Y as tier args or potentially if both are functions of Z one could just pass in Z and compute X and Y from it) |
@wsmoses you mentioned at some point that unlike scf.for, stablehlo.while cannot reference values which are not passed as arguments. Should I update this optimization to only apply to stablehlo.constant ?