Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][CIRGen][NFC] Consolidate RUN lines for builtin tests #968

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

ghehg
Copy link
Collaborator

@ghehg ghehg commented Oct 14, 2024

There is no change to testing functionality. This refacot let those files have the same Run options that is easier to maintain and extend.

@ghehg ghehg marked this pull request as ready for review October 14, 2024 02:04
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for trying to improve things, but I don't think this is the direction to go:

  • These existing builtins tests are actually nicely categorized for what they cover.
  • Most all tests have been designed to contain both CIR and LLVM outputs, I've been asking that for at least 6 months - easier to read and figure out bits all in one place, I don't see a rationale on why this is better.
  • If you want a generic place for builtins you could create a builtin.cpp file for that and add new ones there, or ones that don't seem to require their own file. Note that there's not a single rule on how to go about these.
  • bultins-overflow.cpp could probably be renamed to builtin-overflow.cpp to make names more uniform across the board.

@bcardosolopes
Copy link
Member

Note: if for some reason while unifying things you find out that some don't have LLVM lowering yet, you can workaround by passing a -DFLAG to the -emit-llvm run line and use ifdefs to exclude the specific testcases that aren't yet supported. More or less what's done in clang/test/CIR/CodeGen/three-way-comparison.cpp.

@ghehg ghehg force-pushed the m1Mac branch 2 times, most recently from db4c9f9 to 6c242ed Compare October 15, 2024 23:04
@ghehg ghehg changed the title [CIR][CIRGen][NFC][Builtin][Testing] Consolidate CIR builtin related cpp test files. [CIR][CIRGen][NFC][Builtin][Testing] Refactor CIR builtin related cpp test files Oct 15, 2024
@ghehg
Copy link
Collaborator Author

ghehg commented Oct 15, 2024

Thanks for trying to improve things, but I don't think this is the direction to go:

  • These existing builtins tests are actually nicely categorized for what they cover.
  • Most all tests have been designed to contain both CIR and LLVM outputs, I've been asking that for at least 6 months - easier to read and figure out bits all in one place, I don't see a rationale on why this is better.
  • If you want a generic place for builtins you could create a builtin.cpp file for that and add new ones there, or ones that don't seem to require their own file. Note that there's not a single rule on how to go about these.
  • bultins-overflow.cpp could probably be renamed to builtin-overflow.cpp to make names more uniform across the board.

Just as note, Bruno and I talked, and we repurposed this PR.

@bcardosolopes bcardosolopes changed the title [CIR][CIRGen][NFC][Builtin][Testing] Refactor CIR builtin related cpp test files [CIR][CIRGen][NFC] Consolidate RUN lines for builtin tests Oct 16, 2024
@bcardosolopes bcardosolopes merged commit c0ddc4d into llvm:main Oct 16, 2024
6 checks passed
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
There is no change to testing functionality. This refacot let those
files have the same Run options that is easier to maintain and extend.
lanza pushed a commit that referenced this pull request Nov 5, 2024
There is no change to testing functionality. This refacot let those
files have the same Run options that is easier to maintain and extend.
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