[WIP] Refactor GemmFeatures #2185
Conversation
…il instead of setting a default arch
| GemmFeatures getDefaultFeatures(Type dataType); | ||
|
|
||
| /// Get the default features for multiple types (intersects features) | ||
| GemmFeatures getDefaultFeatures(ArrayRef<Type> types); |
There was a problem hiding this comment.
I thought the idea was to get rid of GemmFeatures? At least that's what I think the end goal should be. Isn't it possible to remove GemmFeatures completely in this PR?
| bool hasAtomicAdd(Type dataType); | ||
|
|
||
| /// Check if f16 atomic add is supported (arch-only) | ||
| bool hasAtomicAddF16() const; |
There was a problem hiding this comment.
nit: hasAtomicAdd(Type) instead of three similar functions.
| bool isMfma(Type dataTypeA, Type dataTypeB); | ||
|
|
||
| /// Check if wmma is supported for given types | ||
| bool isWmma(Type dataTypeA, Type dataTypeB); |
There was a problem hiding this comment.
can we do isAccel() instead of isMfma/isWmma?
| bool isWmma(Type dataTypeA, Type dataTypeB); | ||
|
|
||
| /// Check if direct-to-LDS is supported for given type and numBytes | ||
| bool isDirectToLDS(Type dataType, int64_t numBytes = 0); |
There was a problem hiding this comment.
isn't the dataType enough? why do we need numBytes?
| assert(config.operation.value() == ConvOpType::BwdWeight); | ||
|
|
||
| kernelCount = 1; | ||
| if (isAccel(config.features)) { |
There was a problem hiding this comment.
why do we remove isAccel()?
| return false; | ||
|
|
||
| // Then check type supports direct-to-LDS | ||
| return isDirectToLDS(dataType, numBytes); |
There was a problem hiding this comment.
I think we support (or want to support?) async direct to LDS for 64/8/16 as well?
| return isDirectToLDS(dataType, numBytes); | ||
| } | ||
|
|
||
| bool mlir::rock::AmdArchInfo::hasDot() const { |
There was a problem hiding this comment.
dot is not used anywhere AFAIK, we can get rid of it.
…t expect a certain feature to be not present
Motivation
Technical Details
This PR makes either
archormhal.archattributes mandatory on any kernel. If none are present, lowering will failProblems
Problem 1
Tests where gemm features do not match arch features
Solution: TODO
Problem 2
We use features to specify that a rock.gemm is accel / not accel, but if we are removing gemmfeatures, we lose that ability.
Solution (2 possibilities):
Problem 3
The rocmlir-gen flags that modify the features (now we are getting the features from the arch, so the options do not make sense anymore)
Solution: TODO
TODOs
Test Plan
Test Result
Submission Checklist