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

Mark host_dirty() and device_dirty() with no_discard. #8248

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

mcourteaux
Copy link
Contributor

Lost quite some time, because I needed set_host_dirty() instead of host_dirty().

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

I wish this was the default in C++ :-/

@abadams abadams requested a review from halidebuildbots May 30, 2024 16:45
@mcourteaux
Copy link
Contributor Author

Heh, it's not working... It says "attribute ignored". Why is this? Is the macro not properly defined to check compiler support? Or am I misunderstanding the compiler output?

@steven-johnson
Copy link
Contributor

Heh, it's not working... It says "attribute ignored". Why is this? Is the macro not properly defined to check compiler support? Or am I misunderstanding the compiler output?

Dunno, but for a lark, try putting it before HALIDE_ALWAYS_INLINE?

@mcourteaux
Copy link
Contributor Author

mcourteaux commented May 30, 2024

Ah, it's because you apparently need to put all the attributes in a single [[...]] list... Meh... Let's think...

EDIT: This is not even true... You can do multiple [[ ]] attributes after each other.

@steven-johnson
Copy link
Contributor

Ah, it's because you apparently need to put all the attributes in a single [[...]] list... Meh... Let's think...

We require C++17 at this point, we can just write it out explicitly without a macro

@mcourteaux mcourteaux force-pushed the dirty-must-use-result branch from b8f8e5d to 6a4308b Compare May 30, 2024 17:16
@mcourteaux
Copy link
Contributor Author

Nevermind, you were right the order of the attributes mattered, as we mix __attribute__() with [[...]]. There is no always_inline attribute in for [[ ]] AFAICT, so this will be it for now.

@mcourteaux
Copy link
Contributor Author

mcourteaux commented Jun 1, 2024

Is the cmake issue fixed by this merge of main? If so, the buildbots need to be retriggered. However, some of the builds did run and complete successfully. So, I think this PR is okay, as it's only a C++-level change, and not really anything to do with the logic or workings of Halide.

@steven-johnson steven-johnson merged commit 35143d2 into halide:main Jun 2, 2024
19 checks passed
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