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

[LICMPass] Runtime issues caused by LICMpass optimization #68776

Closed
XingYuShuai opened this issue Oct 11, 2023 · 8 comments
Closed

[LICMPass] Runtime issues caused by LICMpass optimization #68776

XingYuShuai opened this issue Oct 11, 2023 · 8 comments
Labels
loopoptim miscompilation question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!

Comments

@XingYuShuai
Copy link

case:
test-code.txt
Commands:clang -O2 test-code.c && ./a.out 10 VS clang -O0 test-code.c && ./a.out 10
result:
image

@XingYuShuai
Copy link
Author

XingYuShuai commented Oct 11, 2023

IR informations obtained through '-opt-bisect-limit' option:
before run LICMpass:
184-test-code.txt
after run LICMpass:
185-test-code.txt

In this case , the store instruction shouledn't be removed.
image

@nikic
Copy link
Contributor

nikic commented Oct 11, 2023

Infinite loops without side effects are undefined behavior in C. Use -fno-finite-loops to make them well-defined.

@nikic nikic closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2023
@XingYuShuai
Copy link
Author

XingYuShuai commented Oct 12, 2023

Infinite loops without side effects are undefined behavior in C. Use -fno-finite-loops to make them well-defined.

after add option '-fno-finite-loops' ,the result is:
image

And if I changed the case to this:
test-code2.txt

commands changed to :clang -O2 test-code2.c && ./a.out 1 VS clang -O0 test-code2.c && ./a.out 1
the result is also different:
image

In this case , it dose not satisfy the infinit loops.

@nikic

@nikic
Copy link
Contributor

nikic commented Oct 12, 2023

Sorry, I didn't look carefully enough at your example. The issue is even more severe, because you are reading a non-atomic variable from a signal handler.

@XingYuShuai
Copy link
Author

XingYuShuai commented Oct 15, 2023

Sorry, I didn't look carefully enough at your example. The issue is even more severe, because you are reading a non-atomic variable from a signal handler.

Yes, so in LICM, promoteLoopAccessesToScalars function should consider the non-atomic variable readed from a signal handler.
And this issue should be open again?
I have a solution to address this issue.

@nikic
Copy link
Contributor

nikic commented Oct 17, 2023

@XingYuShuai No, your code is not well-defined. You can only read lock-free atomic or volatile sig_atomic_t from a signal handler.

@EugeneZelenko EugeneZelenko added the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label Oct 17, 2023
@XingYuShuai
Copy link
Author

XingYuShuai commented Oct 18, 2023

@XingYuShuai No, your code is not well-defined. You can only read lock-free atomic or volatile sig_atomic_t from a signal handler.
Sorry, I don't fully understand lock-free atomic and volatile sig_atomic_t. But if the Run_Index is volatile or atomic , the StoreSafety in LICM.cpp would not be seted to StoreSafe.
image

XingYuShuai added a commit to XingYuShuai/llvm-project that referenced this issue Oct 18, 2023
being mentioned outside the loop in some cases.

Please refer to issue:68776 for details.

Fixes llvm#68776
Fixes llvm#69279
@XingYuShuai
Copy link
Author

XingYuShuai commented Oct 19, 2023

@XingYuShuai No, your code is not well-defined. You can only read lock-free atomic or volatile sig_atomic_t from a signal handler.

@nikic If you mean the Run_Index is volatile or atomic, in these cases, there is no problem with open source version.
There are my test resultes:
image
test-code2-atomic.txt

test-code2-volatile.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loopoptim miscompilation question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants