[snippy] Add check of duplicated instructions in histogram by Maksim-Sebelev · Pull Request #473 · syntacore/snippy

Maksim-Sebelev · 2026-05-25T10:29:12Z

Hi!

This PR address the following issue - #470

Changes

In each iteration of the

for (auto &&[Opc, WeightRes] : DecodeEntry->Decoded)

we check if Result already contains Opc:

// OpcodeHistogramNormalization::denormalize
      for (auto &&[Opc, WeightRes] : DecodeEntry->Decoded) {
        // FIXME: here we run probability visitor to collect information on
        // which opcodes were already mentioned
        Result.reinitProbabilityVisitor();
        // Check the re-assignment of 'Opc' instruction
        if (Result.contains(Opc)) { // not a problem, because SmallVector
          IO.setError("Incorrect histogram: Redeclaration of " + NameInfo.Val +
                      " weight");
          return {};
        }
        Result.insertTopOpcode(Opc, WeightRes);
      }

Result is a SmallVector, so O(n) complexity (method contains) per iteration isn't a problem. Anyway, this can be just first simple solution.

Verification

A test with a histogram containing a duplicated instruction was also added. This test and all other tests completed successfully on my device.

:)

arromanoff · 2026-05-26T14:29:15Z

+        // FIXME: here we run probability visitor to collect information on
+        // which opcodes were already mentioned
+        Result.reinitProbabilityVisitor();


This call runs expensive visitor so let's not call it inside a loop.
Here Result.contains() would not return true right after Result.insertTopOpcode(Opc, WeightRes). I am not sure how to do this better

Maksim-Sebelev · 2026-05-28T19:14:43Z

I've come up with a smarter way to find duplicates, can you please review it?

Maksim-Sebelev · 2026-05-30T14:51:33Z

I've added the '-Wno-error=duplicated-instructions' option. Could you please review it again?

I haven't documented this yet because I wasn't sure how and where to do it, could you please explain?

About the 4 new similar tests: I can't combine them because the error/warning messages differ by the number of duplicates (there are 2 cases: exactly 1 duplicate, and more than 1 duplicate).

:)

arromanoff · 2026-06-02T14:24:58Z

+sections:
+  - name:      1
+    VMA:       0x210000
+    SIZE:      0x100000
+    LMA:       0x210000
+    ACCESS:    rx
+  - name:      2
+    VMA:       0x400000
+    SIZE:      0x100000
+    LMA:       0x400000
+    ACCESS:    rw


Let's not duplicate this section and just reuse Inputs/section.yaml file

arromanoff · 2026-06-02T14:25:58Z

+  - [LW, 1.0]
+  - [SW, 1.0]
+
+# CHECK: error: (duplicated-instructions) multiple weight definition of DUPLICATED_INSTRUCTION: weight of this instruction will be initialized to the first weight in the histogram.


This looks weird.
Why do we say "weight of this instruction will be initialized to the first weight in the histogram." if it is an error?

By the way, currently, when an opcode appears repeatedly, their weights are summed - this happens after adding histogram-patterns support. For example

histogram-patterns: - AddSub: "ADD | SUB" histogram: - [pattern: AddSub, 1.0] - [ADD, 1.0] - [ADD, 1.0] - [SUB, 1.0]|

In the example above
AddWeight = 1.0 + 1.0 + 1 / 2 (from pattern) = 5 / 2
SubWeight = 1.0 + 1 / 2 (from pattern) = 3 / 2
TotalWeight = AddWeight + SubWeight = 8 / 2 = 4

And here's the question: is this patch even needed? After all, opcodes can now legitimately repeat (given patterns), and the total weight of an opcode is calculated from its weight in the top-level histogram and from patterns.
@arromanoff what do you think?

arromanoff · 2026-06-02T14:28:12Z

+    if (DuplicatedInstructionsCont.empty()) // no duplicates
+      return;
+    std::ostringstream DuplicatedInstructionsMsg;
+    auto DuplicatedInstructionsIt = DuplicatedInstructionsCont.begin();
+    DuplicatedInstructionsMsg << "multiple weight definition of "
+                              << *DuplicatedInstructionsIt++;
+    std::for_each(DuplicatedInstructionsIt, DuplicatedInstructionsCont.end(),
+                  [&DuplicatedInstructionsMsg](std::string_view Instruction) {
+                    DuplicatedInstructionsMsg << ", " << Instruction;
+                  });


We already have llvm::interleaveComma function from STLExtras.h header.
And we don't use standard streams, llvm has its' own raw_string_ostream

arromanoff · 2026-06-02T14:28:39Z

  - [C_LUI_HINT, 4.26]
  - [C_MV, 4.26]
  - [C_MV_HINT, 4.26]
-  - [C_NOP, 4.26]


Redundant change

Redundant change

No, it was a duplicate, so this test has failed after chages from this PR (or I can use here ‘-Wno-error=duplicated-instructions‘ in options)

arromanoff · 2026-06-02T14:29:52Z

  - [C_ADDI, 4.66]
  - [C_ADDIW, 4.66]
  - [C_ADDI_HINT_IMM_ZERO, 4.66]
-  - [C_NOP, 4.66]


redundant change?

I answered it here

arromanoff · 2026-06-02T14:30:43Z

+# RUN: sed -e s/DUPLICATED_INSTRUCTION/ADD/ %s > %t.1.yaml
+# RUN: not llvm-snippy %t.1.yaml --model-plugin=None -mtriple=riscv64 \
+# RUN: |& FileCheck %t.1.yaml
+
+# RUN: sed -e s/DUPLICATED_INSTRUCTION/ADDI/ %s > %t.2.yaml
+# RUN: not llvm-snippy %t.2.yaml --model-plugin=None -mtriple=riscv64 \
+# RUN: |& FileCheck %t.2.yaml


I think these tests are excessive. Can we stop at 3-5 opcodes?

VictorBerbenets · 2026-06-02T15:11:15Z

    Result.reinitProbabilityVisitor();
    return Result;
  }
+  std::unordered_set<std::string_view> UniqueInstructions,


Please try using LLVM types where possible: StringSet instead of unordered_set (I think will be good here), and StringRef instead of string_view

VictorBerbenets · 2026-06-02T15:17:59Z

    return Result;
  }
+  std::unordered_set<std::string_view> UniqueInstructions,
+      DuplicatedInstructions;


It's better to declare one variable per line for readability

VictorBerbenets · 2026-06-02T15:34:04Z

+  - [LW, 1.0]
+  - [SW, 1.0]
+
+# CHECK: error: (duplicated-instructions) multiple weight definition of DUPLICATED_INSTRUCTION: weight of this instruction will be initialized to the first weight in the histogram.


By the way, currently, when an opcode appears repeatedly, their weights are summed - this happens after adding histogram-patterns support. For example

histogram-patterns: - AddSub: "ADD | SUB" histogram: - [pattern: AddSub, 1.0] - [ADD, 1.0] - [ADD, 1.0] - [SUB, 1.0]|

In the example above
AddWeight = 1.0 + 1.0 + 1 / 2 (from pattern) = 5 / 2
SubWeight = 1.0 + 1 / 2 (from pattern) = 3 / 2
TotalWeight = AddWeight + SubWeight = 8 / 2 = 4

And here's the question: is this patch even needed? After all, opcodes can now legitimately repeat (given patterns), and the total weight of an opcode is calculated from its weight in the top-level histogram and from patterns.
@arromanoff what do you think?

Maksim-Sebelev · 2026-06-02T21:34:59Z

Done

VictorBerbenets · 2026-06-03T08:24:09Z

    Result.reinitProbabilityVisitor();
    return Result;
  }
+  StringSet<> UniqueInstructions;


OpcodeHistogram class has TopOpcodes field to keep all top opcodes. Please, use this field and don't create redundant UniqueInstructions

I agree. insertTopNode inserts into TopOpcodes member and we can ask it for contains. This way we don't need to add these UniqueInstructions and DuplicatedInstructions. Yes, we will report one instruction at a time but so be it. After internal discussion we decided that there is no need to report all at once

VictorBerbenets · 2026-06-03T08:28:38Z

        Result.insertTopOpcode(Tgt.getInternalOpcode(Opc), WeightRes);
    }
  }
+  DuplicatedInstructions.begin();


arromanoff · 2026-06-03T08:40:46Z

    Result.reinitProbabilityVisitor();
    return Result;
  }
+  StringSet<> UniqueInstructions;


I agree. insertTopNode inserts into TopOpcodes member and we can ask it for contains. This way we don't need to add these UniqueInstructions and DuplicatedInstructions. Yes, we will report one instruction at a time but so be it. After internal discussion we decided that there is no need to report all at once

Maksim-Sebelev · 2026-06-03T17:50:41Z

Done

VictorBerbenets · 2026-06-04T08:23:19Z

      break;
    }

+    auto &&NameRef = NameInfo.Val;


VictorBerbenets · 2026-06-04T08:25:04Z

+        continue;
+      }
+    }
    auto Name = NameInfo.Val;


No need to create NameRef, you already have Name. Leave just one of them

VictorBerbenets · 2026-06-04T08:27:17Z

+    // FIXME: need to do something when OpcOpt is empty std::optional ???
+    // now it just ignored


NameRef can be the name of the pattern - in this case you pass to OpcodeCache not an Opcode name and it gives you nullopt

Maksim-Sebelev · 2026-06-04T22:42:19Z

Fixed

VictorBerbenets · 2026-06-05T09:05:18Z

In general LGTM

Maksim-Sebelev · 2026-06-06T19:32:26Z

My bad, I've broken my branch. Now it's fixed, the status of the branch is similar to the last commit before this message

arromanoff reviewed May 26, 2026

View reviewed changes

Maksim-Sebelev requested a review from arromanoff May 30, 2026 14:51

Maksim-Sebelev force-pushed the fix/duplicated-instructions branch 6 times, most recently from f37c1f1 to ecc0bf9 Compare May 31, 2026 12:51

arromanoff suggested changes Jun 2, 2026

View reviewed changes

VictorBerbenets reviewed Jun 2, 2026

View reviewed changes

VictorBerbenets reviewed Jun 3, 2026

View reviewed changes

arromanoff suggested changes Jun 3, 2026

View reviewed changes

Maksim-Sebelev force-pushed the fix/duplicated-instructions branch from a82c037 to 3658163 Compare June 3, 2026 18:17

VictorBerbenets reviewed Jun 4, 2026

View reviewed changes

Maksim-Sebelev force-pushed the fix/duplicated-instructions branch from 45c7d95 to 428f80f Compare June 5, 2026 20:52

Maksim-Sebelev closed this Jun 5, 2026

Maksim-Sebelev force-pushed the fix/duplicated-instructions branch from 428f80f to 32ff28f Compare June 5, 2026 21:00

Maksim-Sebelev reopened this Jun 6, 2026

Maksim-Sebelev force-pushed the fix/duplicated-instructions branch from 97ed387 to b9aee52 Compare June 7, 2026 05:58

[snippy] Add check of duplicated instructions

b9aee52

		// FIXME: need to do something when OpcOpt is empty std::optional ???
		// now it just ignored

Conversation

Maksim-Sebelev commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hi!

Changes

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maksim-Sebelev commented May 28, 2026

Uh oh!

Maksim-Sebelev commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VictorBerbenets Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Maksim-Sebelev commented Jun 2, 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!

Choose a reason for hiding this comment

Uh oh!

Maksim-Sebelev commented Jun 3, 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!

Maksim-Sebelev commented Jun 4, 2026

Uh oh!

VictorBerbenets commented Jun 5, 2026

Uh oh!

Maksim-Sebelev commented Jun 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Maksim-Sebelev commented May 25, 2026 •

edited

Loading

Maksim-Sebelev commented May 30, 2026 •

edited

Loading

VictorBerbenets Jun 2, 2026 •

edited

Loading