SROA: speculate load across transitive PHI and SelectInst #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The destructor was virtual but not marked override, causing warnings (and errors in revng if -Wall) was enabled. This commit fixes these problems
`virtual` classes should always have a virtual destructor.
This commit improves the handling of comparision of a value masked in a certain way with a constant. For example, for a 16-bit integer, we might have x & 0xfff0 == 0x1230. This is a typical situation produced by InstCombine to efficiently check if x is between 0x1230 and 0x12340 (excluded).
In file included from /orchestra/sources/llvm/llvm/unittests/ADT/APFixedPointTest.cpp:12:
/orchestra/sources/llvm/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: error: use of overloaded operator '==' is ambiguous (with operand types 'const llvm::APSInt' and 'const unsigned long')
if (lhs == rhs) {
~~~ ^ ~~~
/orchestra/sources/llvm/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<llvm::APSInt, unsigned long>' requested here
return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
^
/orchestra/sources/llvm/llvm/unittests/ADT/APFixedPointTest.cpp:227:3: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<llvm::APSInt, unsigned long>' requested here
ASSERT_EQ(APFixedPoint::getMax(Sema).getIntPart(), Expected);
^
/orchestra/sources/llvm/llvm/utils/unittest/googletest/include/gtest/gtest.h:1956:32: note: expanded from macro 'ASSERT_EQ'
^
/orchestra/sources/llvm/llvm/utils/unittest/googletest/include/gtest/gtest.h:1939:63: note: expanded from macro 'GTEST_ASSERT_EQ'
EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
^
/orchestra/sources/llvm/llvm/include/llvm/ADT/APInt.h:2035:13: note: candidate function (with reversed parameter order)
inline bool operator==(uint64_t V1, const APInt &V2) { return V2 == V1; }
^
/orchestra/sources/llvm/llvm/include/llvm/ADT/APSInt.h:176:8: note: candidate function
bool operator==(int64_t RHS) const {
^
/orchestra/sources/llvm/llvm/include/llvm/ADT/APSInt.h:339:13: note: candidate function (with reversed parameter order)
inline bool operator==(int64_t V1, const APSInt &V2) { return V2 == V1; }
^
1 error generated.
Since C++20, UTF8 string literals (in the form `u8"blabla"`) have type `const char8_t[N]`, which does not play nice with llvm::StringRef and llvm::StringLiteral. This commit fixes compilation of DJBTest.cpp and in C++20 explicitly converting those strings literals to regular string literals. To fix compilation of DJBTest.cpp, we had to define a new UTF8 based constructor for StringRef and one for StringLiteral. These cannot be constexpr because const char8_t * cannot be converted at compile time to const char *. With these new constructors, the StringRef(data, length) constructor becomes ambiguous if used with nullptr as first argument, such as StringRef(nullptr, 0); This was done in XCOFFObjectFile.cpp that was fixed to use the default StringRef constructor.
The exact warning was:
include/clang/AST/DeclarationName.h:210:52: error: arithmetic between different enumeration types ('clang::DeclarationName::StoredNameKind' and 'clang::detail::DeclarationNameExtra::ExtraKind') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
Before this patch, LLVM's DominatorTree and related templates (DominatorTree, DomTreeBuilder) did not support the computation of dominator and post-dominator trees on graphs with markers, such as GraphTraits<llvm::Inverse<T>>. They only supported plain graph traits. This patch restructures all the necessary templates, adding an additional template argument (View), which allows to compute DT and PDT on all the graphs that use GraphTraits, potentially equipped with a View. View is a marker, that allows to provide a different view on a graph that has GraphTraits. A typical example of this is llvm::Inverse. This mechanism of View markers will be used in rev.ng to implement filtered graph traits, to filter the edges of a graph implemented with GraphTraits. The new DT and PDT on the marked version of the GraphTraits will be computed on the same graphs, but treating some edges as if they are seen through the view. For instance, the Post-Dominator Tree of a graph marked with llvm::Inverse view is the Dominator Tree. The previous default behavior of DT and PDT is implemented by means of the DTIdentityView, which simply leaves the node type of the GraphTraits untouched. This is a transparent view that allows all the code in llvm to continue working as before this patch.
These tests pass successfully when running as unprivileged user, but fail if run as `root`, because the superuser has privileges to ignore file permissions. This is a known issue, reported e.g. by Gentoo as well: https://bugs.gentoo.org/775050 For now, disable these tests, since they fail in docker on our CI.
pfez
force-pushed
the
feature/sroa-on-transitive-phis-and-selects
branch
from
ed09bd9 to
1a12679
Compare
tvandera
pushed a commit
to tvandera/llvm-project
that referenced
this pull request
Oct 5, 2021
Andrei Matei reported a llvm11 core dump for his bpf program https://bugs.llvm.org/show_bug.cgi?id=48578 The core dump happens in LiveVariables analysis phase. revng#4 0x00007fce54356bb0 __restore_rt revng#5 0x00007fce4d51785e llvm::LiveVariables::HandleVirtRegUse(unsigned int, llvm::MachineBasicBlock*, llvm::MachineInstr&) revng#6 0x00007fce4d519abe llvm::LiveVariables::runOnInstr(llvm::MachineInstr&, llvm::SmallVectorImpl<unsigned int>&) revng#7 0x00007fce4d519ec6 llvm::LiveVariables::runOnBlock(llvm::MachineBasicBlock*, unsigned int) revng#8 0x00007fce4d51a4bf llvm::LiveVariables::runOnMachineFunction(llvm::MachineFunction&) The bug can be reproduced with llvm12 and latest trunk as well. Futher analysis shows that there is a bug in BPF peephole TRUNC elimination optimization, which tries to remove unnecessary TRUNC operations (a <<= 32; a >>= 32). Specifically, the compiler did wrong transformation for the following patterns: %1 = LDW ... %2 = SLL_ri %1, 32 %3 = SRL_ri %2, 32 ... %3 ... %4 = SRA_ri %2, 32 ... %4 ... The current transformation did not check how many uses of %2 and did transformation like %1 = LDW ... ... %1 ... %4 = SRL_ri %2, 32 ... %4 ... and pseudo register %2 is used by not defined and caused LiveVariables analysis core dump. To fix the issue, when traversing back from SRL_ri to SLL_ri, check to ensure SLL_ri has only one use. Otherwise, don't do transformation. Differential Revision: https://reviews.llvm.org/D97792 (cherry picked from commit 51cdb78)
|
This patch is not necessary anymore. I'm closing this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.