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

Revert "SWDEV-365820 - Refactor build path" #91

Closed
wants to merge 1 commit into from

Conversation

preda
Copy link

@preda preda commented Aug 16, 2024

This reverts commit 3f2f725.

Reason: it breaks -save-temps in OpenCL

See bug report: ROCm/ROCm#2940

This reverts commit 3f2f725.

Reason: it breaks -save-temps in OpenCL

See bug report: ROCm/ROCm#2940
@preda
Copy link
Author

preda commented Aug 25, 2024

Any reviewer's comments?

@preda
Copy link
Author

preda commented Nov 8, 2024

@saleelk hey guys are you serious? your change broke your project, the breakage affects your users, it's a bug, it's been months. Why aren't you reverting the breakage, and take the time to think about the right way to do it without keeping things broken. What are you waiting for? no review yet for the revert -- out of reviewers maybe?

@saleelk
Copy link
Contributor

saleelk commented Dec 10, 2024

Hi @preda , I didnt see this comments as my emails got filtered for "OpenCL" keyword. I have just done a fix in staging, shoudl get reflected here soon! Appreciate your patience

@saleelk
Copy link
Contributor

saleelk commented Dec 10, 2024

we drove this internally as the solution for this should have come from the compiler. but looks like that hasnt happened,

@preda
Copy link
Author

preda commented Dec 13, 2024

@saleek why wait for another project (LLVM?) to fix your breakage? Enough time has passed in the broken state, and that other project didn't fix it as you hoped. The simple solution is to revert your breakage, especially that the revert is just fine. Even the reason invoked for the initial change that broke it, i.e. compilation perf, is probably wrong -- please do time the compilation after the revert, and see whether there is any performance penalty (clearly, in my observation, there isn't). So there is absolutely no reason to keep this change, just revert it already!

@mangupta
Copy link
Contributor

mangupta commented Dec 13, 2024

@preda : Isn't it already reverted for OpenCL path via 23c21d5? Or are you still seeing the issue?

@preda
Copy link
Author

preda commented Dec 13, 2024

@mangupta @saleelk
The change 23c21d5 looks good, thank you. I was not aware of it as the issue that's addressed ROCm/ROCm#2940 was not linked in the commit message.

What about still seeing the issue -- we'll have to wait a bit for the change to be included in the next release, I guess.

@preda
Copy link
Author

preda commented Dec 13, 2024

Closing this PR because fully addressed by 23c21d5

@preda preda closed this Dec 13, 2024
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.

3 participants