Skip to content

Conversation

@badumbatish
Copy link
Contributor

Fixes #1849

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

High-level comments

@bcardosolopes bcardosolopes changed the title Add dedicated CIR op for setjmp [CIR] Add dedicated CIR op for setjmp Aug 22, 2025
Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

A few more nits on the description. A little confused by the current one.

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.

Getting closer, thanks for the updates!

Seems like this PR is missing the CIRGen change that emits cir.eh.setjmp, can you please add it as part of this PR?

@badumbatish
Copy link
Contributor Author

Getting closer, thanks for the updates!

Seems like this PR is missing the CIRGen change that emits cir.eh.setjmp, can you please add it as part of this PR?

yep on it

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

test_setjmp2 function in both test files needs to be updated as well. It should codegen a cir.eh.setjmp ... (w/o builtin)

Once that's done, it would lgtm

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

Pretty close, I think there might be a bug in the tests and 1 formatting nit

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

lgtm

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.

LGTM after applying comments from @xlauko

@bcardosolopes bcardosolopes merged commit 4e092b2 into llvm:main Sep 5, 2025
8 of 9 checks passed
tommymcm pushed a commit to tommymcm/clangir that referenced this pull request Sep 5, 2025
bcardosolopes added a commit that referenced this pull request Sep 5, 2025
@bcardosolopes
Copy link
Member

Had to revert because it broke test on Linux:

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.

[CIR] Add dedicated setjmp op

5 participants