[mlir][dxsa] Refine operands by tagolog · Pull Request #164 · access-softek/llvm-project

tagolog · 2026-06-12T02:57:58Z

Example:
dxsa.add r<0>, r<1>, r<2>
dxsa.add_sat r<0>, r<1>, r<2>
dxsa.add r<0>, r<1>, l(1.0, 2.0, 3.0, 4.0)
dxsa.add r<0>, -r<1>, |r<2>|

asl · 2026-06-12T06:54:49Z

+      DXSA_SrcOperandAttr:$rhs,
+      OptionalAttr<DXSA_ComponentMaskAttr>:$precise);
+  let results = (outs);
+  let assemblyFormat = "$dst `,` $lhs `,` $rhs attr-dict";


assemblyFormat does not use $precise. Unused?

asl · 2026-06-12T07:01:37Z

+    return success();
+  }
+  if (auto intAttr = llvm::dyn_cast<IntegerAttr>(attr)) {
+    outBits = intAttr.getValue().getZExtValue();


This asserts in the case of value not fitting into 64 bits.

asavonic

LGTM overall. With "default" attributes hidden, text format doesn't look too verbose.

asavonic · 2026-06-12T14:24:43Z

+      canonical count for the operand type.
+    - `min16f` / `min2_8f` / `min16i` / `min16u` - the
+      `#dxsa.operand_min_precision` hint.
+    - `<chans>` - positional channel list, parsed as a


Perhaps rename <chans> to <mask>?

The whole operand documentation has been reafctored.

asavonic · 2026-06-12T14:25:57Z

+  StringRef text(out.data(), out.size());
+  if (text.contains('.'))
+    return out;
+  size_t exponentPos = text.find_first_of("eE");


I think you can instead use LLVM write_double function with FloatStyle::Fixed style.

asavonic · 2026-06-12T14:26:30Z

+  case 4:
+    return OperandComponents::vector;
+  default:
+    return OperandComponents::reserved;


Do we actually need this? Can we do anything with "reserved" number of components other than to emit an error?

asavonic · 2026-06-12T14:36:17Z

+/// Parses one literal element of an `l(...)` / `d(...)` payload. Accepts a
+/// float literal, a decimal integer, or a hex integer, and reinterprets the
+/// value as the raw bit pattern at the requested width.
+static ParseResult parseImmValue(AsmParser &parser, bool is64,


Do we need floating point immediate here at all? IIRC, a type is not encoded, just bit size.

The original disassembler prints them as integer and floats, but I think it does this by looking at the corresponding instruction. If an immediate operand is used in an FP instruction, then it is printed as FP. I don't think we have to do the same here.

The point is, to avoid dancing around different formats, we can support only integer format (or hex - whatever format MLIR can parse by default).

asl · 2026-06-12T18:20:55Z

+    <x>
+    ```
+  }];
+  let parameters = (ins ArrayRefParameter<"int64_t">:$channels);


why int64_t? Can it be, say, -1? If not, why not just plain `unsigned?

asl · 2026-06-12T18:33:50Z

+  if (auto intAttr = llvm::dyn_cast<IntegerAttr>(attr)) {
+    auto width = is64 ? 64u : 32u;
+    auto value = intAttr.getValue();
+    if (value.getBitWidth() < width) {


Does this mean that it would automatically accept, say 42 : i13 and just drop the type promoting to 32-bit int? Is sign-extension always desired? Say, we're having 0x7FFFFFFF: i31, you will extend this to 0xFFFFFFFF. Is this ok?

asl · 2026-06-12T18:36:48Z

+}
+
+template <typename Float, typename UInt>
+static bool floatSpellingReparsesToIdenticalBits(Float value, UInt bits) {


This is terrible.... Do we really need this?

asl · 2026-06-12T18:37:18Z

+  imm.bits = bits;
+  imm.byteWidth = sizeof(UInt);
+
+  // Positive zero is the only value spelled as a bare `0`; every other pattern,


Why do you need to guess in the IR? This is not in general user-written code. IR should be precise.

asl · 2026-06-12T18:38:13Z

+//===----------------------------------------------------------------------===//
+
+/// Parses the body of a 32-bit immediate-literal operand.
+ParseResult mlir::dxsa::parseImm32Body(AsmParser &parser,


Why do you need to export all these functions? Aren't these internal business of the parser?

asl · 2026-06-12T18:42:29Z

+
+/// Parses a single channel keyword `x`, `y`, `z` or `w`.
+static ParseResult parseChannel(AsmParser &parser,
+                                SmallVectorImpl<int64_t> &channels) {


Why it takes vectors and pushes to it rather than returning item inderectly? This seems to differ from existing parser API.

asl · 2026-06-12T18:43:44Z

+  return channel == 0 ? 'x' : channel == 1 ? 'y' : channel == 2 ? 'z' : 'w';
+}
+
+static std::optional<int64_t> channelFromKeyword(StringRef keyword) {


Do you really need to wrap integer into optional? It seems to be used only once, fold into parseChannel and you will be able to return error directly.

Oh, btw, and there is llvm::StringSwitch for you.

asl · 2026-06-12T18:46:12Z

+}
+
+/// Parses an index entry that begins with the already-parsed integer `value`,
+/// e.g. `2`, `2 : i64`, `2 : i64 + r<...>`.


Do you really allow, say, i17? It seems you silently ignore it.

asl · 2026-06-12T18:47:12Z

+void IndexAttr::print(AsmPrinter &printer) const {
+  if (auto imm = getImm()) {
+    printer << imm.getValue();
+    if (imm.getType().isInteger(64))


Aren't you hardcode everything to i32 during parsing (in parseIntIndexEntry)?

asl · 2026-06-12T19:29:49Z

+    mask = ComponentMaskAttr::get(ctx, ComponentMask::x | ComponentMask::y |
+                                           ComponentMask::z | ComponentMask::w);
+
+  return parser.getChecked<DstOperandAttr>(loc, ctx, *operandType, components,


Parsing is great, but for mere mortals you'd probably add some additional attr builders that would automatically add defaults

asl · 2026-06-12T19:30:43Z

+          return {};
+
+        applySrcOperandDefaults(ctx, *type, body);
+        return parser.getChecked<SrcOperandAttr>(


See comment about about default attribute builders

asl · 2026-06-12T19:32:27Z

+dxsa.add r<0>, cb<2 : i64 + r<3, <x>>>, r<2>
+
+// -----
+
+// CHECK: dxsa.add r<0>, cb<[1 : i64, 2 + r<3, <x>>, r<5, <y>>]>, r<2>
+dxsa.add r<0>, cb<[1 : i64, 2 + r<3, <x>>, r<5, <y>>]>, r<2>


you're allowing other types (e.g. i17 and i43). You need either disallow them or test

asl

See all comments above

asl · 2026-06-12T19:33:00Z

@@ -0,0 +1,47 @@
+// RUN: mlir-opt %s -split-input-file | FileCheck %s


you should use --verify-roundtrip in all dialect syntax / op tests

Example: dxsa.add r<0>, r<1>, r<2> dxsa.add_sat r<0>, r<1>, r<2> dxsa.add r<0>, r<1>, l(1.0, 2.0, 3.0, 4.0) dxsa.add r<0>, -r<1>, |r<2>| Signed-off-by: Vladimir Shiryaev <tagolog@users.noreply.github.com>

tagolog requested review from asavonic and asl June 12, 2026 02:58

asl reviewed Jun 12, 2026

View reviewed changes

asavonic reviewed Jun 12, 2026

View reviewed changes

tagolog force-pushed the dxsa-mlir-refine-operands branch from ce14854 to 2960121 Compare June 12, 2026 18:09

asl reviewed Jun 12, 2026

View reviewed changes

asl requested changes Jun 12, 2026

View reviewed changes

tagolog force-pushed the dxsa-mlir-refine-operands branch 2 times, most recently from 74a9f0e to d70f9e9 Compare June 12, 2026 22:07

[mlir][dxsa] Refine operands

6c5bf58

Example: dxsa.add r<0>, r<1>, r<2> dxsa.add_sat r<0>, r<1>, r<2> dxsa.add r<0>, r<1>, l(1.0, 2.0, 3.0, 4.0) dxsa.add r<0>, -r<1>, |r<2>| Signed-off-by: Vladimir Shiryaev <tagolog@users.noreply.github.com>

tagolog force-pushed the dxsa-mlir-refine-operands branch from d70f9e9 to 6c5bf58 Compare June 12, 2026 22:44

		@@ -0,0 +1,47 @@
		// RUN: mlir-opt %s -split-input-file \| FileCheck %s

Conversation

tagolog commented Jun 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asavonic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants