From b5a8a355a6531d3a0f2982c871ca71063c1fc6c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Warzy=C5=84ski?= Date: Wed, 19 Feb 2025 13:02:13 +0000 Subject: [PATCH 1/4] Extend TestingGuide (adds notes on formatting) --- .../content/getting_started/TestingGuide.md | 245 ++++++++++++++++++ 1 file changed, 245 insertions(+) diff --git a/website/content/getting_started/TestingGuide.md b/website/content/getting_started/TestingGuide.md index a889b66d7118..b6609741a16e 100644 --- a/website/content/getting_started/TestingGuide.md +++ b/website/content/getting_started/TestingGuide.md @@ -372,3 +372,248 @@ func.func @simple_constant() -> (i32, i32) { } ``` +### Test Formatting Best Practices + +When adding new tests, strive to follow these two key rules: + +1. **Follow the existing naming and whitespace style.** + - This applies when modifying existing test files. +2. **Consistently document the edge case being tested.** + - Clearly state what makes this test unique and how it complements other + similar tests. + +While the first rule extends LLVM’s general coding style to tests, the second +may feel new. The goal is to improve: + +- **Test discoverability** – Well-documented tests make it easier to pair tests + with patterns and understand their purpose. +- **Test consistency** – Consistent documentation and naming lowers cognitive + load and helps avoid duplication. + +A well-thought-out naming convention helps achieve all of the above. + +--- + +#### Example: Improving Test Readability & Naming + +Consider these **three tests** that exercise `vector.maskedload -> vector.load` +lowering under the `-test-vector-to-vector-lowering` flag: + +##### Before: Inconsistent & Hard to Differentiate + +```mlir +// CHECK-LABEL: func @maskedload_regression_1( +// CHECK-SAME: %[[A0:.*]]: memref, +// CHECK-SAME: %[[A1:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[A0]][%[[C]]] : memref, vector<16xf32> +// CHECK: return %[[LOAD]] : vector<16xf32> +func.func @maskedload_regression_1(%arg0: memref, %arg1: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %vec_i1 = vector.constant_vec_i1 [16] : vector<16xi1> + + %ld = vector.maskedload %arg0[%c0], %vec_i1, %arg1 + : memref, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} + +// CHECK-LABEL: func @maskedload_regression_2( +// CHECK-SAME: %[[A0:.*]]: memref<16xi8>, +// CHECK-SAME: %[[A1:.*]]: vector<16xi8>) -> vector<16xi8> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[A0]][%[[C]]] : memref<16xi8>, vector<16xi8> +// CHECK: return %[[LOAD]] : vector<16xi8> +func.func @maskedload_regression_2(%arg0: memref<16xi8>, %arg1: vector<16xi8>) -> vector<16xi8> { + %c0 = arith.constant 0 : index + %vec_i1 = vector.constant_vec_i1 [16] : vector<16xi1> + + %ld = vector.maskedload %arg0[%c0], %vec_i1, %arg1 + : memref<16xi8>, vector<16xi1>, vector<16xi8> into vector<16xi8> + + return %ld : vector<16xi8> +} + +// CHECK-LABEL: func @maskedload_regression_3( +// CHECK-SAME: %[[A0:.*]]: memref<16xf32>, +// CHECK-SAME: %[[A1:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: return %[[A1]] : vector<16xf32> +func.func @maskedload_regression_3(%arg0: memref<16xf32>, %arg1: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %vec_i1 = vector.constant_vec_i1 [0] : vector<16xi1> + + %ld = vector.maskedload %arg0[%c0], %vec_i1, %arg1 + : memref<16xf32>, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} +``` +While all examples test `vector.maskedload` -> `vector.load lowering`, it’s +difficult to tell their actual differences. + +##### Step 1: Use Consistent Variable Names + +To reduce cognitive load, use consistent names across MLIR and FileCheck. Also, +instead of using generic names like `%arg0`, encode some additional context by +using names from existing documentation (e.g. from Op definition, see e.g. +[vector.maskedload](https://mlir.llvm.org/docs/Dialects/Vector/#vectormaskedload-vectormaskedloadop) +for this particular case): + +```mlir +// CHECK-LABEL: func @maskedload_regression_1( +// CHECK-SAME: %[[BASE:.*]]: memref, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[BASE]][%[[C]]] : memref, vector<16xf32> +// CHECK: return %[[LOAD]] : vector<16xf32> +func.func @maskedload_regression_1(%base: memref, %pass_thru: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [16] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} + +// CHECK-LABEL: func @maskedload_regression_2( +// CHECK-SAME: %[[BASE:.*]]: memref<16xi8>, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xi8>) -> vector<16xi8> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[BASE]][%[[C]]] : memref<16xi8>, vector<16xi8> +// CHECK: return %[[LOAD]] : vector<16xi8> +func.func @maskedload_regression_2(%base: memref<16xi8>, %pass_thru: vector<16xi8>) -> vector<16xi8> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [16] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref<16xi8>, vector<16xi1>, vector<16xi8> into vector<16xi8> + + return %ld : vector<16xi8> +} + +// CHECK-LABEL: func @maskedload_regression_3( +// CHECK-SAME: %[[BASE:.*]]: memref<16xf32>, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: return %[[PASS_THRU]] : vector<16xf32> +func.func @maskedload_regression_3(%base: memref<16xf32>, %pass_thru: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [0] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref<16xf32>, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} +``` + +##### Step 2: Improve Test Naming + +Instead of using "regression" (which doesn't add unique information), rename +tests based on key attributes: + +* All examples test the `vector.maskedload` to `vector.load` lowering. +* The first test uses a _dynamically_ shaped `memref`, while the others use + _static_ shapes. +* The mask in the first two examples is "all true" (`vector.constant_mas + [16]`), while it is "all false" (`vector.constant_mask [0]`) in the thir + example. +* The first and the third tests use `i32` elements, whereas the second uses + `i8`. + +This suggest the following naming scheme: +* `@maskedload_to_load_{static|dynamic}_{i32|i8}_{all_true|all_false}`. + +```mlir +// CHECK-LABEL: func @maskedload_to_load_dynamic_i32_all_true( +// CHECK-SAME: %[[BASE:.*]]: memref, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[BASE]][%[[C]]] : memref, vector<16xf32> +// CHECK: return %[[LOAD]] : vector<16xf32> +func.func @maskedload_to_load_dynamic_i32_all_true(%base: memref, %pass_thru: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [16] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} + +// CHECK-LABEL: func @maskedload_to_load_static_i8_all_true( +// CHECK-SAME: %[[BASE:.*]]: memref<16xi8>, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xi8>) -> vector<16xi8> { +// CHECK: %[[C0:.*]] = arith.constant 0 : index +// CHECK: %[[LOAD:.*]] = vector.load %[[BASE]][%[[C]]] : memref<16xi8>, vector<16xi8> +// CHECK: return %[[LOAD]] : vector<16xi8> +func.func @maskedload_to_load_static_i8_all_true(%base: memref<16xi8>, %pass_thru: vector<16xi8>) -> vector<16xi8> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [16] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref<16xi8>, vector<16xi1>, vector<16xi8> into vector<16xi8> + + return %ld : vector<16xi8> +} + +// CHECK-LABEL: func @maskedload_to_load_static_i32_all_false( +// CHECK-SAME: %[[BASE:.*]]: memref<16xf32>, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: return %[[PASS_THRU]] : vector<16xf32> +func.func @maskedload_to_load_static_i32_all_false(%base: memref<16xf32>, %pass_thru: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [0] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref<16xf32>, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} +``` + +##### Step 3: Add The Newly Identified Missing Case + +The update in Step 2 made us realize that we were missing an important edge +case: + +* A mask that is neither "all true" nor "all false". + +Unlike the existing cases, this mask must be preserved. In this scenario, +`vector.load` is not the right abstraction. Thus, no lowering should occur: + +```mlir +// CHECK-LABEL: func @negative_maskedload_to_load_static_i32_mixed( +// CHECK-SAME: %[[BASE:.*]]: memref<16xf32>, +// CHECK-SAME: %[[PASS_THRU:.*]]: vector<16xf32>) -> vector<16xf32> { +// CHECK: vector.maskedload +func.func @negative_maskedload_to_load_static_i32_mixed(%base: memref<16xf32>, %pass_thru: vector<16xf32>) -> vector<16xf32> { + %c0 = arith.constant 0 : index + %mask = vector.constant_mask [4] : vector<16xi1> + + %ld = vector.maskedload %base[%c0], %mask, %pass_thru + : memref<16xf32>, vector<16xi1>, vector<16xf32> into vector<16xf32> + + return %ld : vector<16xf32> +} +``` + +The `negative_` prefix indicates that this test should fail to lower, as the +pattern should not match. + +To summarize, here’s the naming convention used: + +* `@{negative_}?maskedload_to_load_{static|dynamic}_{i32|i8}_{all_true|all_false|mixed}`. + +#### Final Points - key principles + +The above approach is just an example. It may not fit your use case perfectly, +so feel free to adapt it as needed. For example, for "folding" tests, it may +make more sense to use "no" as prefix for negative tests (e.g. +`@no_fold__`). Key principles to follow: + +* Make tests self-documenting. +* Follow existing conventions. + +By applying these best practices, we leverage available tools (e.g., test +function names) to make tests easier to discover and maintain. From aa27c9df6037f15977c2b8c9c5b360351e279b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Warzy=C5=84ski?= Date: Thu, 20 Feb 2025 11:09:53 +0000 Subject: [PATCH 2/4] fixup! Extend TestingGuide (adds notes on formatting) Fix formatting, add paragraph on what to do when there's no (good) pre-exisiting style to follow. --- .../content/getting_started/TestingGuide.md | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/website/content/getting_started/TestingGuide.md b/website/content/getting_started/TestingGuide.md index b6609741a16e..6dd3bf7b7c6a 100644 --- a/website/content/getting_started/TestingGuide.md +++ b/website/content/getting_started/TestingGuide.md @@ -448,6 +448,7 @@ func.func @maskedload_regression_3(%arg0: memref<16xf32>, %arg1: vector<16xf32>) return %ld : vector<16xf32> } ``` + While all examples test `vector.maskedload` -> `vector.load lowering`, it’s difficult to tell their actual differences. @@ -456,7 +457,7 @@ difficult to tell their actual differences. To reduce cognitive load, use consistent names across MLIR and FileCheck. Also, instead of using generic names like `%arg0`, encode some additional context by using names from existing documentation (e.g. from Op definition, see e.g. -[vector.maskedload](https://mlir.llvm.org/docs/Dialects/Vector/#vectormaskedload-vectormaskedloadop) +[`vector.maskedload`](https://mlir.llvm.org/docs/Dialects/Vector/#vectormaskedload-vectormaskedloadop) for this particular case): ```mlir @@ -516,7 +517,7 @@ tests based on key attributes: * The first test uses a _dynamically_ shaped `memref`, while the others use _static_ shapes. * The mask in the first two examples is "all true" (`vector.constant_mas - [16]`), while it is "all false" (`vector.constant_mask [0]`) in the thir + [16]`), while it is "all false" (`vector.constant_mask [0]`) in the third example. * The first and the third tests use `i32` elements, whereas the second uses `i8`. @@ -605,7 +606,19 @@ To summarize, here’s the naming convention used: * `@{negative_}?maskedload_to_load_{static|dynamic}_{i32|i8}_{all_true|all_false|mixed}`. -#### Final Points - key principles +#### What if there is no pre-existing style to follow? + +If you are adding a new test file, you can use other test files in the same +directory as inspiration. + +If the test file you are modifying lacks a clear style and instead has mixed, +inconsistent styles, try to identify the dominant one and follow it. Even +better, consider refactoring the file to adopt a single, consistent style — +this helps improve our overall testing quality. + +This is also encouraged when the existing style leaves room for improvement. + +#### Final Points - Key Principles The above approach is just an example. It may not fit your use case perfectly, so feel free to adapt it as needed. For example, for "folding" tests, it may From f362ae2cf93e8572f3eddb574135a2aa8bb1c4de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Warzy=C5=84ski?= Date: Thu, 20 Feb 2025 11:13:14 +0000 Subject: [PATCH 3/4] fixup! fixup! Extend TestingGuide (adds notes on formatting) Add a section on documentaiton --- .../content/getting_started/TestingGuide.md | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) diff --git a/website/content/getting_started/TestingGuide.md b/website/content/getting_started/TestingGuide.md index 6dd3bf7b7c6a..7fb8678a6b6c 100644 --- a/website/content/getting_started/TestingGuide.md +++ b/website/content/getting_started/TestingGuide.md @@ -630,3 +630,205 @@ make more sense to use "no" as prefix for negative tests (e.g. By applying these best practices, we leverage available tools (e.g., test function names) to make tests easier to discover and maintain. + +### Test Docmentation Best Practices + +Apart from following good naming and formatting conventions, please document +your tests with comments. Focus on explaining **why** rather than **what**, +since the latter is usually clear from the code itself. + +As an example, consider two test cases exercising the same pattern: + + +```mlir +// CHECK-LABEL: func.func @xfer_write_minor_identity_transposed +// CHECK-SAME: %[[VEC:.*]]: vector<4x8xi16>, +// CHECK-SAME: %[[MEM:.*]]: memref<2x2x8x4xi16> +// CHECK-SAME: %[[IDX:.*]]: index) +// CHECK: %[[TR:.*]] = vector.transpose %[[VEC]], [1, 0] : vector<4x8xi16> to vector<8x4xi16> +// CHECK: vector.transfer_write + +/// The permutation map was replaced with vector.transpose +// CHECK-NOT: permutation_map + +// CHECK-SAME: %[[TR]], %[[MEM]][%[[IDX]], %[[IDX]], %[[IDX]], %[[IDX]]] : vector<8x4xi16>, memref<2x2x?x?xi16> +func.func @xfer_write_minor_identity_transposed( + %vec: vector<4x8xi16>, + %mem: memref<2x2x8x4xi16>, + %idx: index) { + + vector.transfer_write %vec, %mem[%idx, %idx, %idx, %idx] { + in_bounds = [true, true], + permutation_map = affine_map<(d0, d1, d2, d3) -> (d3, d2)> + } : vector<4x8xi16>, memref<2x2x8x4xi16> + + return +} + +/// Even with out-of-bounds accesses, it is safe to apply this pattern as it +/// does not modify the memory location being accessed. + +// CHECK-LABEL: func.func @xfer_write_minor_identity_transposed_out_of_bounds +// CHECK-SAME: %[[VEC:.*]]: vector<4x8xi16> +// CHECK-SAME: %[[MEM:.*]]: memref<2x2x?x?xi16> +// CHECK-SAME: %[[IDX:.*]]: index) +// CHECK: %[[TR:.*]] = vector.transpose %[[VEC]], [1, 0] : vector<4x8xi16> to vector<8x4xi16> + +/// Expect the in_bounds attribute to be preserved. However, since we don't +//// print it when all flags are "false", it should not appear in the output. +/// CHECK-NOT: in_bounds + +// CHECK: vector.transfer_write + +/// The permutation map was replaced with vector.transpose +// CHECK-NOT: permutation_map + +// CHECK-SAME: %[[TR]], %[[MEM]][%[[IDX]], %[[IDX]], %[[IDX]], %[[IDX]]] : vector<8x4xi16>, memref<2x2x?x?xi16> +func.func @xfer_write_minor_identity_transposed_out_of_bounds( + %vec: vector<4x8xi16>, + %mem: memref<2x2x?x?xi16>, + %idx: index) { + + vector.transfer_write %vec, %mem[%idx, %idx, %idx, %idx] { + in_bounds = [false, false], + permutation_map = affine_map<(d0, d1, d2, d3) -> (d3, d2)> + } : vector<4x8xi16>, memref<2x2x?x?xi16> + + return +} +``` + +Here, we document two non-obvious behaviors: + +* _Why_ is `permutation_map` missing from the output? +* _Why_ is the `in_bounds` attribute missing in one of the outputs? + + +#### How to Identify What Needs Documentation? +A good rule of thumb is to think of yourself six months from now and ask: +"What might be difficult to decipher without comments?" + +If you expect something to be tricky for future-you, it’s likely to be tricky +for others encountering the test for the first time. + +#### Making Tests Self-Documenting +We can improve documentation further by clarifying what pattern is being +tested, providing high-level reasoning, and consolidating shared comments. For +instance: + +```mlir +///---------------------------------------------------------------------------------------- +/// [Pattern: TransferWritePermutationLowering] +/// +/// IN: vector.transfer_write (_transposed_ minor identity permutation map) +/// OUT: vector.transpose + vector.transfer_write (minor identity permutation map) +/// +/// Note, `permutation_map` from the input Op is replaced with the newly +/// inserted vector.traspose Op. +///---------------------------------------------------------------------------------------- + +// CHECK-LABEL: func.func @xfer_write_minor_identity_transposed +// CHECK-SAME: %[[VEC:.*]]: vector<4x8xi16>, +// CHECK-SAME: %[[MEM:.*]]: memref<2x2x8x4xi16> +// CHECK-SAME: %[[IDX:.*]]: index) +// CHECK: %[[TR:.*]] = vector.transpose %[[VEC]], [1, 0] : vector<4x8xi16> to vector<8x4xi16> +// (...) +``` + +This documents: +* The transformation pattern being tested. +* The key logic behind the transformation. +* The expected change in output. + + +#### Documenting the "What" +While we generally document why, documenting what is valid and encouraged in +cases where: + +* The he test generates long or complex output +* The logic is non-trivial or involves multiple transformations + +For example, in a test for Linalg convolution vectorization, some comments +document what is happening step-by-step: + +```mlir +func.func @conv1d_nwc_4x2x8_memref(%input: memref<4x6x3xf32>, %filter: memref<1x3x8xf32>, %output: memref<4x2x8xf32>) { + linalg.conv_1d_nwc_wcf + {dilations = dense<1> : tensor<1xi64>, strides = dense<3> : tensor<1xi64>} + ins(%input, %filter : memref<4x6x3xf32>, memref<1x3x8xf32>) + outs(%output : memref<4x2x8xf32>) + return +} + +// CHECK: #[[INPUT_MAP:.+]] = affine_map<(d0, d1, d2, d3) -> (d0, d1, d3)> +// CHECK: #[[FILTER_MAP:.+]] = affine_map<(d0, d1, d2, d3) -> (d3, d2)> +// CHECK: #[[OUTPUT_MAP:.+]] = affine_map<(d0, d1, d2, d3) -> (d0, d1, d2)> + +// CHECK: func @conv1d_nwc_4x2x8_memref +// CHECK-SAME: (%[[INPUT:.+]]: memref<4x6x3xf32>, %[[FILTER:.+]]: memref<1x3x8xf32>, %[[OUTPUT:.+]]: memref<4x2x8xf32>) + +// CHECK-DAG: %[[C0:.+]] = arith.constant 0 : index +// CHECK-DAG: %[[F0:.+]] = arith.constant 0.000000e+00 : f32 + +/// Read the whole data in one shot. +// CHECK-DAG: %[[V_INPUT_R:.+]] = vector.transfer_read %[[INPUT]][%[[C0]], %[[C0]], %[[C0]]], %[[F0]] +// CHECK-DAG: %[[V_FILTER_R:.+]] = vector.transfer_read %[[FILTER]][%[[C0]], %[[C0]], %[[C0]]], %[[F0]] +// CHECK-DAG: %[[V_OUTPUT_R:.+]] = vector.transfer_read %[[OUTPUT]][%[[C0]], %[[C0]], %[[C0]]], %[[F0]] + +// CHECK: %[[V_INPUT_0:.+]] = vector.extract_strided_slice %[[V_INPUT_R]] +// CHECK-SAME: {offsets = [0, 0, 0], sizes = [4, 1, 3], strides = [1, 1, 1]} : vector<4x4x3xf32> to vector<4x1x3xf32> +// CHECK: %[[V_INPUT_1:.+]] = vector.extract_strided_slice %[[V_INPUT_R]] +// CHECK-SAME: {offsets = [0, 3, 0], sizes = [4, 1, 3], strides = [1, 1, 1]} : vector<4x4x3xf32> to vector<4x1x3xf32> + +// CHECK: %[[V_FILTER:.+]] = vector.extract %[[V_FILTER_R]][0] : vector<3x8xf32> from vector<1x3x8xf32> + +// CHECK: %[[V_OUTPUT_0:.+]] = vector.extract_strided_slice %[[V_OUTPUT_R]] +// CHECK-SAME: {offsets = [0, 0, 0], sizes = [4, 1, 8], strides = [1, 1, 1]} : vector<4x2x8xf32> to vector<4x1x8xf32> +// CHECK: %[[V_OUTPUT_1:.+]] = vector.extract_strided_slice %[[V_OUTPUT_R]] +// CHECK-SAME: {offsets = [0, 1, 0], sizes = [4, 1, 8], strides = [1, 1, 1]} : vector<4x2x8xf32> to vector<4x1x8xf32> + +/// w == 0, kw == 0 +// CHECK: %[[CONTRACT_0:.+]] = vector.contract { +// CHECK-SAME: indexing_maps = [#[[INPUT_MAP]], #[[FILTER_MAP]], #[[OUTPUT_MAP]]], +// CHECK-SAME: iterator_types = ["parallel", "parallel", "parallel", "reduction"] +// CHECK-SAME: kind = #vector.kind +// CHECK-SAME: %[[V_INPUT_0]], %[[V_FILTER]], %[[V_OUTPUT_0]] +// CHECK-SAME: : vector<4x1x3xf32>, vector<3x8xf32> into vector<4x1x8xf32> + +/// w == 1, kw == 0 +// CHECK: %[[CONTRACT_1:.+]] = vector.contract { +// CHECK-SAME: indexing_maps = [#[[INPUT_MAP]], #[[FILTER_MAP]], #[[OUTPUT_MAP]]], +// CHECK-SAME: iterator_types = ["parallel", "parallel", "parallel", "reduction"] +// CHECK-SAME: kind = #vector.kind +// CHECK-SAME: %[[V_INPUT_1]], %[[V_FILTER]], %[[V_OUTPUT_1]] +// CHECK-SAME: : vector<4x1x3xf32>, vector<3x8xf32> into vector<4x1x8xf32> + +/// w == 0, kw == 0 +// CHECK: %[[RES_0:.+]] = vector.insert_strided_slice %[[CONTRACT_0]], %[[V_OUTPUT_R]] +// CHECK-SAME: {offsets = [0, 0, 0], strides = [1, 1, 1]} : vector<4x1x8xf32> into vector<4x2x8xf32> +/// w == 1, kw == 0 +// CHECK: %[[RES_1:.+]] = vector.insert_strided_slice %[[CONTRACT_1]], %[[RES_0]] +// CHECK-SAME: {offsets = [0, 1, 0], strides = [1, 1, 1]} : vector<4x1x8xf32> into vector<4x2x8xf32> + +/// Write the result back in one shot. +// CHECK: vector.transfer_write %[[RES_1]], %[[OUTPUT]][%[[C0]], %[[C0]], %[[C0]]] + +``` + +Note that while the comments document what is happening (e.g., "Write the +result back in one shot"), some variables — like `w` and `kw` — remain +"enigmatic" and are not explicitly explained. This might leave us +second-guessing their meaning at first. However, that’s intentional—their +purpose becomes clear when studying the corresponding Linalg vectorizer +implementation. + +It’s important to remember that comments should assist in understanding the +code, not replace the need to read it. Their role is to guide the reader, not +to restate what the code already conveys. + +#### Final Takeaways +* Prioritize documenting _why_ something happens, unless the _what_ is non-trivial. +* Use high-level block comments to describe patterns being tested. +* Think about maintainability - comments should help future developers + understand tests at a glance. +* Comments should assist, not replace reading the code—avoid over-explaining. From 6f7b7697ac7911f935fa9db047a4f6e594b6fe06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Warzy=C5=84ski?= Date: Sun, 23 Feb 2025 14:39:13 +0000 Subject: [PATCH 4/4] fixup! fixup! fixup! Extend TestingGuide (adds notes on formatting) Fix typos/formatting, add paragraph on common sense --- website/content/getting_started/TestingGuide.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/website/content/getting_started/TestingGuide.md b/website/content/getting_started/TestingGuide.md index 7fb8678a6b6c..9e21880a3b0e 100644 --- a/website/content/getting_started/TestingGuide.md +++ b/website/content/getting_started/TestingGuide.md @@ -516,7 +516,7 @@ tests based on key attributes: * All examples test the `vector.maskedload` to `vector.load` lowering. * The first test uses a _dynamically_ shaped `memref`, while the others use _static_ shapes. -* The mask in the first two examples is "all true" (`vector.constant_mas +* The mask in the first two examples is "all true" (`vector.constant_mask [16]`), while it is "all false" (`vector.constant_mask [0]`) in the third example. * The first and the third tests use `i32` elements, whereas the second uses @@ -618,6 +618,16 @@ this helps improve our overall testing quality. This is also encouraged when the existing style leaves room for improvement. + +#### Don't forget the common sense + +As a reviewer or contributor, always apply common sense when naming functions +and variables. Encoding too much information in names can negatively impact +readability and maintainability. + +Trust your judgment. When in doubt, consult your future self: "Will this still +make sense to me six months from now?" + #### Final Points - Key Principles The above approach is just an example. It may not fit your use case perfectly, @@ -830,5 +840,5 @@ to restate what the code already conveys. * Prioritize documenting _why_ something happens, unless the _what_ is non-trivial. * Use high-level block comments to describe patterns being tested. * Think about maintainability - comments should help future developers - understand tests at a glance. + understand tests at a glance. * Comments should assist, not replace reading the code—avoid over-explaining.