-
Notifications
You must be signed in to change notification settings - Fork 44
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
Revert "Revert "[frontend] Remove Complex Regex for MLIR Parsing (#4924)"" #2681
base: main
Are you sure you want to change the base?
Conversation
…24)"" This reverts commit ecc9bd4.
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
python/triton/compiler/compiler.py
Outdated
target = driver.active.get_current_target() | ||
backend = make_backend(target) | ||
backend.load_dialects(context) |
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.
The main change is that for our backend, parse_mlir_module
function only works correctly with the corresponding dialects.
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.
Why DPAS layout requires additional dialects while MMA layout doesn't?
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.
MMA layout belongs to triton_gpu
dialect:
return f"#{GPU_DIALECT}.nvidia_mma<{{versionMajor={self.version[0]}, versionMinor={self.version[1]}, warpsPerCTA={self.warps_per_cta}, CTAsPerCGA={self.ctas_per_cga}, CTASplitNum={self.cta_split_num}, CTAOrder={self.cta_order}, instrShape={self.instr_shape}}}>" |
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.
Dpas explicitly redefines the dialect (that implicitly comes from DistributedEncoding
IIUC):
Lines 12 to 13 in 0187e36
def DpasEncodingAttr : DistributedEncoding<"DpasEncoding", "intel_dpas_encoding", | |
[MmaEncodingTrait], TritonIntelGPU_Dialect> { |
context = ir.context() | ||
ir.load_dialects(context) | ||
backend.load_dialects(context) | ||
|
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.
This is different from upstream code:
Yes, I made this change on purpose. Now, as far as I understand, the backend dialects should be initialized when calling the IRSource
class constructor.
Also, would adding the else part eliminate the need to add lines 100-103 ?
No, because IRSource
constructor also calls function self.module = ir.parse_mlir_module(self.path, context)
, which requires the backend dialects.
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.
One example, we use dpas layout that requires an additional dialect:
DpasLayout(repeatCount=8, systolic_depth=8, execution_size=8, ops_per_chan=1, threads_per_warp=32, |
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.
Can this change be upstreamed?
python/triton/compiler/compiler.py
Outdated
target = driver.active.get_current_target() | ||
backend = make_backend(target) | ||
backend.load_dialects(context) |
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.
Why DPAS layout requires additional dialects while MMA layout doesn't?
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
I think there is a chance. I brought it to a form more similar to what I am trying to upstream. @etiotto @whitneywhtsang could we merge it? |
@anmyachev what is the upstream PR for this changes ? Can you open an issue to track upstreaming the changes as well please ? |
Closes #2653
This reverts commit ecc9bd4.
The issue looks like: