Repository navigation
[dependencies] Install torch-mlir from PyPI - #302
Conversation
|
torch-mlir behavior has changed in the latest version:
|
ce0911b to
2a2e567
Compare
|
Rebased on current main pulling in #298. Making the new Most importantly, in the attention lowering we can no longer call the
Without the elementwise fusion the attention graph is now more complex with smaller ops. Several generalizations were implemented in The fastmath flag was also changed from |
charithaintc
left a comment
There was a problem hiding this comment.
added some comments to clear some initial doubts.
| result, and R2 reduces E's result. The loop step supplies the tile size. | ||
| result, and R2 reduces E's result, possibly through elementwise intermediates. | ||
| The loop step supplies the tile size. Intermediates are inlined into a | ||
| temporary consumer for analysis only. In the fused loop they are tiled as |
There was a problem hiding this comment.
is this temp consumer dead code? is that removed after analysis?
I think proper way to handle additional elementwise ops would be to fuse them into a single E. did you consider using fine grained elementwise fusion extension for this?
There was a problem hiding this comment.
is this temp consumer dead code? is that removed after analysis?
Yes. We create a temporary fused op to allow easier analysis of the graph, and it is being explicitly removed. The benefit is that we can reuse the existing legality checks without a need to write new analysis methods for the non-fused case (e.g. proving linearity or tracing indexing maps).
I think proper way to handle additional elementwise ops would be to fuse them into a single E. did you consider using fine grained elementwise fusion extension for this?
The caveat here is the -inf masking. We actually do not want to fuse the arith.select op into E. Therefore, in case of a masked op, we always have a graph E -> zero-select -> PV and handling this pattern would require more complex analysis in any case. That said, there certainly are different ways of implementing this.
| """Create the fusion transform and return its new loop handle.""" | ||
| """Fuse the chain using apply-time payload analysis. | ||
|
|
||
| Recognized softmax uses stable correction, assumes finite auxiliary inputs |
There was a problem hiding this comment.
In follow up PR, we should try to remove this specialization for softmax. I think this ok for now given the complexity of the changes.
There was a problem hiding this comment.
Yes, this transform now has softmax specific handling. We essentially need two guards in the fused loop:
- In the E op we need to set
exp(-inf - (-inf))to0to guard against producing NaNs. - Replace the correction term
term(a) / term(b)with stablea == -inf ? 0 : exp(a + b). I.e. two guards, first against-infand second one forexp(a+b)stability.
These are not generic guards but depend on E being a softmax. I agree this is not ideal. In principle it could be possible to pull these guards out of the fuse_dependent_reduction_ops transform into a separate softmax specific transform that runs after. It would be somewhat complex because we'd need to trace the IR, check for softmax and match the appropriate handles etc.
I also note that convert-math-to-xevm pass converts exp(a)/exp(b) to exp(a-b) later on in the pipeline so for xegpu this guard is not needed here.
I did experiment with dropping these guards and the KB attention layer kernel executes correctly. So one options is to drop these in this PR and handle them later.
There was a problem hiding this comment.
I also note that convert-math-to-xevm pass converts exp(a)/exp(b) to exp(a-b) later on in the pipeline so for xegpu this guard is not needed here.
OK. I think I have a better understanding now.
As you mentioned already we reply on fastmath flags to do the reassociation. Without fastmath this is not a valid rewrite ( exp(a)/exp(b) to exp(a-b)). Honestly I don't think we need to guard against it. user needs to add reassoc flag and legality check should check them before fusion in my view.
| class RewriteAttentionRowMaskOp( | ||
| TransformExtensionDialect.Operation, name="rewrite_attention_row_mask" | ||
| ): | ||
| """Replace all--inf row masks using guarded online attention's maximum. |
There was a problem hiding this comment.
can you explain how this works?
several questions:
- does this run after or before dependent red fusion? if after fusion then how is the AND reduction fused>
- does this remove the AND reduction?
There was a problem hiding this comment.
Yes, this transform runs after the row-sum and PV fuse_dependent_reduction_ops transforms. It's purpose is to fuse the scores == -inf mask computation to the reduction loop by re-using the already existing max reduction (note that all(scores == -inf) is equivalent to max(scores) == -inf):
- In the reduction loop, check whether the running max reduction is
-inf. - After the reduction loop, check whether the final maximum is
-inf. With this update we do not need theandreduction anymore and it can be dropped.
This is a specialized transform. The benefit is that it's kept separate from the more generic fuse_dependent_reduction_ops op.
There was a problem hiding this comment.
okay. it makes sense.
for future: Do you think there will be a way to fuse the scores == -inf similar to how we fuse others here. maybe converting it to a OR reduction of score != -inf.
There was a problem hiding this comment.
That could be a possible way to handle and reductions, let's assess these ideas once this PR has landed.
| sum accumulator is ``exp(-m_new) / exp(-m_old)`` before adding this contribution | ||
| of current tile ``sum(exp(x_tile - m_new))``. | ||
|
|
||
| Recognized softmax chains use ``exp(m_old - m_new)`` and zero weights for |
There was a problem hiding this comment.
it seems this removes the dependency of row mask on PV? and moves it after the fused loop.
it also looks like code is specialized for the softmax case now. this is less desirable. but I guess we can think of a proper solution in another PR.
There was a problem hiding this comment.
I also wonder if we can do the RewriteAttentionRowMaskOp can be applied before fusion so that PV does not directly depend on the mask output. instead we compute the mask normally and do mask application after the fused loop.
There was a problem hiding this comment.
I also wonder if we can do the RewriteAttentionRowMaskOp can be applied before fusion so that PV does not directly depend on the mask output. instead we compute the mask normally and do mask application after the fused loop.
This kind of design might be possible, but simply computing the mask from the final max before dependent-reduction fusion is insufficient. The in-loop PV computation needs a predicate based on the updated running max, so fusion would need to remap that dependency. Also, moving the mask application after PV is not generally equivalent: an all--inf intermediate prefix can produce NaNs in the running computation even when the final row contains finite scores.
There was a problem hiding this comment.
okay. I am still missing something here.
all(scores == -inf) whole point of this is to avoid that row in P participating in PV, right? otherwise things would underflow.
My suggestion was to not use running max at all. instead we do the all(scores == -inf) reduction normally inside the fused loop but fused PV does not use it. PV will possibly will overflow in certain rows.
Then after the loop we rescale PV with the output of all(scores == -inf). this will mask out rows of PV where P had all -inf row. Maybe my understanding is not correct. but good to verify if this approach works or now.
(btw even if this works we don't have a general way to fuse the AND reduction in first place. so its an open problem)
There was a problem hiding this comment.
This is slightly more subtle. On the global untiled kernel level, we indeed only need to compute the full -inf rows and we can apply that mask after PV matmul to mask out those invalid values.
However, this changes when we apply online reduction to the softmax. Now, if the first n values in that row are -inf, the partial max is -inf and softmax gives exp(-inf - -inf) = NaN. This NaN persists and propagates to the output, even if the later values were finite. Thus opting for online reduction exposes a new NaN generation mechanism(!). And if we want to guard against these NaNs as well, we need to update the mask computation accordingly, i.e. update it similarly with online reduction.
That's why we have both rewrite_attention_row_mask transform (in the reduction loop, it uses partial max_reduction to mask the p tile, and after the loop also applies the mask to the final result) and the softmax-specific guarding in fuse_dependent_reduction_ops.
charithaintc
left a comment
There was a problem hiding this comment.
LGTM. may require some clenups/redesigns in follow up PRs.
thanks for clearing the misunderstandings.
This is required with attn layer in newer torch-mlir version 20261001.
If attention layer has a P row mask, we need to rewrite the mask usage in PV contraction to be able to fuse the mask generation in the reduction loop.
…ms referenced by their inputs This allows CSE to eliminate identical factors in the graph.
This should be replaced by adding bindings to linalg::populateFoldReshapeOpsByExpansionPatterns.
We cannot call linalg-fuse-elementwise-ops pass because the p mask must be kept separate from the pv matmul to be able to map the latter to a xegpu.dpas op. Skipping the monolithic fusion, the graph now has more small ops and lowering logic needs to be updated. - fuse_dependent_reduction_ops.py: _prepare_consumer still builds an inlined copy of R2, but only to run the legality and correction-factor checks. It then deletes that copy and passes the real R2 plus the list of in-between elementwise ops to the loop builder. Keeps the mask separate from the pv matmul. - dependent_reduction_fusion.py: _chain_tiled_dims works out which loop dims of each in-between op follows R2's reduction axis, and rejects the fusion if that isn't a plain dimension. The in-between ops are cloned in reduction loop and R2 reads the result of the last op. - attention_row_mask.py: handles non-fused attention IR, e.g. the mask select and -inf compare as separate ops, zero as a dense constant tensor.
This should be replaced by upstream FoldScalarOrSplatConstant pattern.
Also move hoisting to a better position
Necessary so that masked attn can be lowered to binary
torch-mlir now keeps the intermediate values at f32 so we have a truncf op between xegpu load and dpas
e4501d2 to
6d06898
Compare
Addresses #283.
Assisted-by: Claude, GPT