[NFC] meaningful naming for aggregate types#9347
[NFC] meaningful naming for aggregate types#9347peterbell10 merged 7 commits intotriton-lang:mainfrom
Conversation
peterbell10
left a comment
peterbell10
left a comment
There was a problem hiding this comment.
Looks really useful, could you add a test though?
| return value, cursor | ||
|
|
||
| def _flatten_ir_types(self, builder: ir.builder, out: List[ir.type]) -> None: | ||
| ## FOUND TENSOR DESCRIPTOR TYPE HERE |
| ) | ||
|
|
||
| def _set_name(self, builder: ir.builder, name: str) -> None: | ||
| self.handle.set_loc(builder.create_name_loc(name, self.handle.get_loc())) |
There was a problem hiding this comment.
Can you do this in tensor_descriptor_base?
|
|
||
| def _set_name(self, builder: ir.builder, name: str) -> None: | ||
| for i, v in enumerate(self.values): | ||
| v._set_name(builder, f"{name}.{i}") |
There was a problem hiding this comment.
Can you use the field names for named tuples?
|
Thanks @peterbell10 ! I think I have addressed your feedback. Let me know if you see anything else. |
| type: base_type | ||
|
|
||
| def _set_name(self, builder: ir.builder, name: str) -> None: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Test failures are real. Might be worth having a default implementation?
There was a problem hiding this comment.
Hmm, looks like the formatter added a line and threw off the line numbers.
I don't have an AMD gpu but thought I found all the derivations from base_value.. I'll track these down.
I could have the base_value implementation do what the old impl did, calling _flatten_ir.. That would be reasonable for any unknown derivations. But having the assert also forces those to think about naming.
|
Apparently, not entirely fixed.. investigating |
|
Tests cleaned up, no longer perturbs line numbers.
|
Generate `<name>.<field>` for aggregate fields, including `<index>` for tuples.
- address review feedback - added test
- update test for line_num changes
- self-contained tests for tensordesc (only on cuda) and namedtuples
sjw36
force-pushed
the
sjw/codegen-names
branch
from
6b9ab39 to
b8c4c56
Compare
|
Also disabled test for @peterbell10 Is there a reason tensordesc codegen cannot be uniform across all architectures? |
|
tensordesc is backed by a hardware feature which not all devices have, so it has a fallback that decomposes it. |
|
Right, I'm mostly wondering why the function params cannot uniformly be codegen'd to:
Then later architectures that do not support TDM could just convert the Currently it generates the same fields anyway in I guess there should be some way to associate all related params to a tensordesc as well, so that the runtime can programmatically specify the arguments. This could also lead to removing unused args. I can put up a PR to this effect if it makes sense. |
This reverts commit 2b1031d.
2 participants
Generate
<name>.<field>for aggregate fields, including<index>for tuples.This change adds a
_set_namemethod tobase_valueand all derived types.NOTE: this could impact language extensions.