-
Notifications
You must be signed in to change notification settings - Fork 10
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
chlo.lgamma const prop #182
base: main
Are you sure you want to change the base?
Conversation
if(!matchPattern(op.getOperand(),m_Constant(&inputAttr))) | ||
return failure(); | ||
|
||
Value result = materializeLgamma(rewriter,op.getLoc(),op->getOperands()); |
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.
so this is fun, but stablehlo at least has a eval method with the literal constant implemented. Apparently stablehlo doesn;t =/.
Since this code looks like it comes from lowerchlotostablehlo, maybe we can just run the relevant lowerchlo function here if it has a constant operand (rather than copying the lowering here).
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.
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, but now that I look at it, it's marked static
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.
Would be happy to expose the functions in some shareable way! Could make a header ChloDecompositionUtils.h
and can expose the individual decomp pattern, or the materialize function of interest.
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.
That would be helpful! It'd also be good to have a single implementation for the function definition
We want to perform constant propogation through `chlo.lgamma` in Enzyme-JaX [Kevin](EnzymeAD/Enzyme-JAX#182 (comment)) mentioned he was open to exposing some materialize functions (which are currently static, and not callable from [our pass](https://github.com/EnzymeAD/Enzyme-JAX/blob/main/src/enzyme_ad/jax/Passes/EnzymeHLOOpt.cpp) atm) @wsmoses @GleasonK
@vimarsh6739 anything blocking here (I think we've since updated to a stablehlo version with the interface)? |
Not really, I'll cleanup and update the PR in a bit(need to add a lit test) |
0c5dd3a
to
58443b8
Compare
So, interestingly, it seems like some other optimization is messing with
After re-enabling all other patterns though, I get this.
Currently trying to sweep through the list of emitted ops, but would appreciate pointers as to why |
If you run it in gdb can you tell which one is triggering. another option is you can pass —debug and it will spew for every transform |
Haven't enabled debug symbols(laptop) but seems to crash in
|
which is again strange as the expanded op doesn't have a |
cc @avik-pal |
Ok, disabling it works for now - @avik-pal you can use the above example as a test case. |
@vimarsh6739 does #216 fix it for you? |
let me check |
that works |
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, but maybe it would make sense to add the log, is_inf, and log_plus_one constprop ones first (so this test just becomes a single const return)?
Yep, will add those as a separate PR. Let's hold off on merging for now then. |
|
for #179