Skip to content

Update TableGen for Qwerty_BasisVectorTreeAttr#48

Open
akshaydaf wants to merge 7 commits into
gt-tinker:wip/rec-vecfrom
akshaydaf:updatereps
Open

Update TableGen for Qwerty_BasisVectorTreeAttr#48
akshaydaf wants to merge 7 commits into
gt-tinker:wip/rec-vecfrom
akshaydaf:updatereps

Conversation

@akshaydaf

Copy link
Copy Markdown
Collaborator

No description provided.

@ausbin ausbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you test this?

Is it only tested that it compiles, or did you try executing this new code somehow? (The easiest way is probably a FileCheck test)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow great idea for how to approach this. I was wondering how to do it, but you answered it.

It just needs a robust verifier, which you added (thanks)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to write a test to test at least the happy path for this parsing and verification. (In the past, when I haven't created such a test, the code breaks later when testing something tangential, confusing the daylights out of me)

Here's the existing parsing test file for the Qwerty dialect that you can extend with a few new tests to hit the happy paths: https://github.com/gt-tinker/qwerty/blob/main/qwerty_mlir/tests/Qwerty/IR/parsing.mlir. Note in the first line it basically says to parse the IR, print it, and then parse it again.

The tricky part is going to be writing the attribute at all, since I don't think there are any ops right now that have this attribute. Hmmm...

The other tricky part is that FileCheck can be pretty confusing. It is type (4) of tests described in this document: https://github.com/gt-tinker/qwerty/blob/main/docs/testing.md. I can explain it in a meeting if it's helpful. I am not aware of there being a good tutorial for writing FileCheck. I just had to stare at other people's tests and the documentation to figure it out, but I can try to save you time by skipping most of that pain

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, here's how upstream tests attribute parsing: https://github.com/llvm/llvm-project/blob/9f73a97cfb1a40da42a870906e2221102a71f807/mlir/test/IR/custom-float-attr-roundtrip.mlir#L6. That's pretty cool, they have a dummy op called test.op we can use. I just checked and the test dialect seems to be included in our LLVM build

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this in a slightly different way by just using a module declaration. Let me know if that works for you, I was able to run the run-tests.sh script and it seemed to pass.

@akshaydaf akshaydaf changed the base branch from main to wip/rec-vec June 10, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants