[xls][mlir] Fix emission of debug locations in xls_translate

[schilkp]

As noted in #1916, I was not able to easily add a round-trip test for channel attributes due to some issues with debug locations in xls_translate.

While fixing this, I wanted to verify that round-trip conversion worked by smoke testing with ops_translate.mlir, where I noticed that my xls->mlir translation did not support trace and that my naming of the fifo flop kind enum was inconsistent.

---

Poping the stack, this PR contains the following commits addressing all of the above:

[xls][mlir] Use underscore in zero latency enum case name

Makes it consistent with all other values in XLS MLIR assembly.

[xls][mlir] Add xls->mlir translation of trace IR node

[xls][mlir] Emit well-formed XLS source location infos in xls_translate

xls_translate previously assigned an ID to every source file encountered in the MLIR Location of input ops and referenced these IDs in the XLS pos attribute of the produced IR nodes, but did not actually add these source file names to the XLS package, leading to invalid pos attributes.

This patch fixes this by relying on Package::AddSourceLocation to both construct the SourceLocation for nodes and automatically registering the file with the package.

With this, xls_translate can also re-use the file name ID assignment/lookup from XLS proper.

[xls][mlir] Add round-trip conversion test for channel properties

This was previously not easily possible due to the incorrect emission of XLS file locations in xls_translate.

[xls][mlir] Expand ops_translate.mlir test to round-trip verification.

---

I rolled all this into a single PR because these are all interdependent, probably making review a bit more annoying. Sorry 😇

@jpienaar @jmolloy

[ericastor]

@jpienaar @jmolloy

[jpienaar]

Nice, I'd have had trace as separate change (just make it easier to see in git history). Could you split it out?

[schilkp]

Nice, I'd have had trace as separate change (just make it easier to see in git history). Could you split it out?

I have it as a separate commit in this PR (one of five) - Do you need it as a separate PR because this somehow gets squashed while merging?

[jpienaar]

I have it as a separate commit in this PR (one of five) - Do you need it as a separate PR because this somehow gets squashed while merging?

Yes indeed (each PR becomes one commit in the history). The rest actually fits well together in one. [well and if I do it manually I think only one gets attributed to you, which is not accurate]

[schilkp]

Yes indeed (each PR becomes one commit in the history). The rest actually fits well together in one. [well and if I do it manually I think only one gets attributed to you, which is not accurate]

Got it. I had a PR¹ with three commits land a few weeks ago, and "from the outside" they seemed to stay separate²³⁴ - so I assumed that was fine and got lazy 👼

Copybara works in mysterious ways.

Will fixup + rebase tomorrow.

Footnotes

1. https://github.com/google/xls/pull/1945 ↩

2. https://github.com/google/xls/commit/16636a3977a96f8927588366b25997d6a27757c9 ↩

3. https://github.com/google/xls/commit/9db4c8377626caf2217e5832b68df1f3b1a6a80f ↩

4. https://github.com/google/xls/commit/206d65af672d4d1a7685dd8e6ddfe038bc8155b1 ↩

[schilkp]

Rebased + trace op conversion removed. Note that I also had to drop the round-trip testing because that will not work without trace. That will also follow in a different PR.

Ready from my side.

[schilkp]

See also:

• [xls][mlir] Add xls->mlir translation of trace IR node #2004
• [xls][mlir] Expand round-trip testing #2005

Note that #2005 relies on both this PR and #2004 for CI to pass.

[schilkp]

If you did, loc("arg") , you could verify the name makes it to XLS and back.

Turns out it does not - I will open a different PR with the fix and that additional test.
Good catch :)

Turns out it was a simple fix. I rolled it into this PR because it seems relevant enough.

[jpienaar]

Good, one extra fix rolled in :-)