[Mlir-commits] [mlir] [mlir] Handle repeated EmitC operands (PR #206001)

[Gil Rapaport]

https://github.com/aniragil commented:

Thanks for addressing this issue @mygitljf !

The bug is that the firstIndexOf each unique operand gets is its index in the original expression, not its index in the unique expression's. A simpler/smaller/immediate fix would be to just fix the `i` count:

```C++
    int i = 0;
    for (auto operand : expressionOp.getDefs()) {
      if (uniqueOperands.contains(operand))
        continue;
      uniqueOperands.insert(operand);
      firstIndexOf[operand] = i++;
    }
```

Your new mapping is more elegant than the indexed one, but since it's it's not a trivial fix as the above but replacing part of the mechanism it's hard to understand from the PR description what the bug was. So it would be great if you either:

- Commit the (above) more direct fix to the bug in this PR (assuming you agree it fixes the bug), then open another PR for replacing the indexed mechanism with your simpler mapping, or

- Keep this PR as-is, but elaborate in the PR description and commit message about the bug and about the new mapping that replaces the index-based mechanism, so future readers can easily follow the rationale.

it's not a major rewrite, so I'm ok with either option.


https://github.com/llvm/llvm-project/pull/206001