Skip to content

Conversation

@maksimsab
Copy link
Contributor

@maksimsab maksimsab commented Jan 11, 2022

Usually clang with SimplifyCFG pass optimizes constructs like:

if (i % 2 == 0)
  func();
else
 func();

into one simple func() invocation.
This behaviour might be wrong in cases when func's behaviour depends on
a place where it is written. For this cases we turn off part of optimizations of SimplifyCFG pass.
There is a relevant discussion about introducing a reliable tool for such cases: https://reviews.llvm.org/D85603

@maksimsab maksimsab requested review from a team as code owners January 11, 2022 14:38
@maksimsab
Copy link
Contributor Author

/summary:run

@ghost
Copy link

ghost commented Jan 11, 2022

@maksimsab Why is this change marked as [NFC]?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, would it be better to simply add a "complete switch off" option to the pass itself? Something similar to what is done in #5149?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that. It seemed for me that compiler should behave as usual in case when sycl isn't involved (when -fsycl option is not present).

Copy link
Contributor

Choose a reason for hiding this comment

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

It seemed for me that compiler should behave as usual in case when sycl isn't involved (when -fsycl option is not present).

So the idea is to have that new switch off by default, i.e. by default the pass is working as-is. Under SYCL mode we will turn it on. Anyway, it seems to me that we can minimize amount of changes by simply disabling hoisting common instructions which is only done once (see my other comment)

@AlexeySachkov
Copy link
Contributor

@maksimsab Why is this change marked as [NFC]?

I guess technically, it is supposed to be a non-functional change, because optimizations must not alter the functionality. But the tag on this PR still makes me a bit nervous :)

@maksimsab maksimsab changed the title [SYCL][NFC] Turn off SimplifyCFG pass in SYCL mode. [SYCL] Turn off SimplifyCFG pass in SYCL mode. Jan 12, 2022
@keryell
Copy link
Contributor

keryell commented Jan 13, 2022

I guess technically, it is supposed to be a non-functional change, because optimizations must not alter the functionality. But the tag on this PR still makes me a bit nervous :)

I prefer a lacking [NFC] to a misleading [NFC]. :-)

Copy link

@xtian-github xtian-github left a comment

Choose a reason for hiding this comment

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

Alexey, thanks for creating this new patch. This patch has a better trade-off than previous one. Is it feasible to disable SimplifyCFG for “common call instructions” for SYCL? If it will take a long time to do so. I am ok for you to get your current patch merged first.

@bader
Copy link
Contributor

bader commented Jan 20, 2022

Is it feasible to disable SimplifyCFG for “common call instructions” for SYCL? If it will take a long time to do so. I am ok for you to get your current patch merged first.

It should be feasible to disable host/sink of "convergent" instructions. I think it's not as simple as config pass update, but we can investigate this option.

@maksimsab, please fix linter complains to unblock further pre-commit testing.

@maksimsab maksimsab force-pushed the turn_off_simplifycfg branch from 3b13d88 to 80b1377 Compare January 26, 2022 12:20
mlychkov
mlychkov previously approved these changes Jan 27, 2022
@bader
Copy link
Contributor

bader commented Jan 31, 2022

@maksimsab, #5305 has been merged. Please, update your branch.

@bader bader requested a review from AlexeySachkov January 31, 2022 12:51
@bader
Copy link
Contributor

bader commented Jan 31, 2022

/summary:run

@maksimsab maksimsab changed the title [SYCL] Turn off SimplifyCFG pass in SYCL mode. [SYCL] Turn off part of SimplifyCFG optimizations in SYCL mode. Feb 2, 2022
@bader bader merged commit 8b29220 into intel:sycl Feb 2, 2022
@maksimsab maksimsab deleted the turn_off_simplifycfg branch December 7, 2022 15:30
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.

8 participants