This patch enhances hasAddressTaken() to ignore bitcasts as a
callee in callbase instruction. Such bitcast usage doesn't really take
the address in a useful meaningful way.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 | Should add test that is a callee operand. Also need to add test where it is an argument in a call. | |
| llvm/test/Analysis/CallGraph/ignore-bitcast-callees.ll | ||
|---|---|---|
| 12 | remove #1, here and below. | |
Comment Actions
Users can just call stripPointerCasts first.No need to add a special case here(which is also incoplete)
Comment Actions
This test also does not really work until you add true argument to the call graph builder.
Comment Actions
I'm not sure it's worth having this be an option. You can just always do this, or not
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1628 | Also should restrict to constant expression case. Also need to verify it is a call operand | |
Comment Actions
For AMDGPU target, we want to add a bunch of attributes to those functions which are indirectly called. To call a function indirectly, the programmer first needs to capture its address in a function pointer and use the function pointer in a call instruction. Many times the address is taken but not really stored anywhere and thus not used meaningfully or in any way which can be de-referenced. One such example is usage of bitcasts in call instruction as shown in the test case. hasAddressTaken() API is useful for the purpose but we want ignore such “false” address taken cases whose the address is not really used. This patch adds an optional flag to ignore those cases. As it stands today, hasAddressTaken() returns "true" for the test case, which is correct but skipping the test scenario is useful for us.
I hope this clarifies.
Comment Actions
I am not sure if I follow. I don't intend do any changes (at least for current patch) in the way call graph builder works because that will break a lot of things. (I tried adding "true" for call graph builder and call changed which was obvious.) FileCheck commands are just inline with what this opt with this patch would emit. Referring to https://reviews.llvm.org/D96087, would those test cases too won't work?
Comment Actions
Idea is fine. We don't want a flag here though.
Bitcasts of a function pointer that are only used as *callees* do not take the address of the function pointer.
Comment Actions
The problem is that test does not exercise your code unless it is enabled, so it does not test anything.
Remove the option though and do it always, a direct call through bitcast does not take function's address. That will make test functional as well.
Also a second call to the same bitcast which will test that all_of() in case it has more than one use.
Comment Actions
rebase, address review comments to remove the optiona flag, update summary, rename and fix tests.
| llvm/include/llvm/IR/Function.h | ||
|---|---|---|
| 887 | Revert this change, you are not touching this file anymore. | |
| llvm/test/Analysis/CallGraph/ignore-bitcast-call-argument-callee.ll | ||
| 12 | I do not like that CG does not see a call to foo, but that's probably a separate change. | |
| llvm/test/Analysis/CallGraph/ignore-bitcast-callees.ll | ||
| 16 | Can you add a second identical call to the same bitcast? So that you have test where the same bitcast is used twice. | |
Comment Actions
I updated it but I don't why phab does not show it. Here is my new commit message:
[IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction
This patch enhances hasAddressTaken() to ignore bitcasts as a
callee in callbase instruction. Such bitcast usage don't really take
the address in a useful meaningful way.
Differential Revision: https://reviews.llvm.org/D98884/
Comment Actions
There are a lot of tests failing as shown by the buildbot on this page. I tried to fix a couple of them but to fix some of them I need to change the code as well. For example, to fix Transforms/GlobalOpt/evaluate-call.ll, I should fix casting in ChangeCalleesToFastCall in GlobalOpt.cpp because cast<CallBase>(U) fails as the user is now BitCastOperator. For more details, you can see the backtrace reported by the buildbot.
I am thinking of reintroducing the flag again because this change may lead to unknown territory and surprising behaviour for some users. Flag would minimize the impact. We should remove the flag as a separate change.
What do you think?
Comment Actions
I'm totally with @arsenm here. You will have to adjust the users to cope with this, arguably it improves our analysis and optimization capabilities and is the entire point of this patch.
Actually looking at the effect also minimizes the risk of corner cases we forgot.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2149 ↗ | (On Diff #332898) | Now we could even have a helper of some sort. Unsure what design is best. |
| llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll | ||
| 5 ↗ | (On Diff #332898) | Hm, if you just run the update script the tests should be properly updated, did you do that? |
| llvm/test/Transforms/Attributor/IPConstantProp/arg-type-mismatch.ll | ||
| 5 ↗ | (On Diff #332898) | Same as above but I do not understand why this change would cause us to loose the function attributes if no AA is provided. |
Comment Actions
use script to update the test, add utility function and address other parts of global opt to fix clang tests
Comment Actions
@jdoerfert This patch is blocking https://reviews.llvm.org/D99347 for AMDGPU. Please have a look.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2135–2139 ↗ | (On Diff #333088) | Just use cast<> |
| 2167–2171 ↗ | (On Diff #333088) | Just use cast<> |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2167–2171 ↗ | (On Diff #333088) | Are you suggesting to remove if (!CB) code block? If so, how would it work in release builds? If cast<> fails, in debug builds, the assert would fire but in release builds, the code will try to access the member function because we don't have "return" statement. Does failure on cast<> return? |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2167–2171 ↗ | (On Diff #333088) | cast<> asserts in a debug build. dyn_cast is when you are trying to handle the null case |
Comment Actions
I'm looking and it seems there is more content added every revision which makes it a moving target.
Also, if you don't get feedback, it is custom to wait for a week to ping: https://www.llvm.org/docs/Contributing.html#how-to-submit-a-patch
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2135–2139 ↗ | (On Diff #333088) | This can at some point error out. Why would the user of ChangeCalleesToFastCall and others need to require this property? |
| 2156 ↗ | (On Diff #333088) | This part was not in the original patch and is now tagged along. At the very least add it to the commit message. It is also unclear why you chose these two functions, are there no other users() loops? |
| 2185 ↗ | (On Diff #333088) | The function is called remove... and the helper set... |
| llvm/test/Analysis/CallGraph/ignore-bitcast-call-argument-callee.ll | ||
| 13 | I run this with trunk and the output looks the same. What exactly is this testing? | |
| llvm/test/Transforms/GlobalOpt/bitcast-callees-fastcc.ll | ||
| 17 ↗ | (On Diff #333088) | Also unclear what is tested here? Looks like the trunk output. |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2135–2139 ↗ | (On Diff #333088) | I think I am missing something here. This function is capturing common logic used by ChangeCalleesToFastCall. |
| 2156 ↗ | (On Diff #333088) | This fix is needed because GlobalOpt/evaluate-call-errors.ll failed with an assert as now OptimizeFunctions() is making a call to this function because of the change in return value by hasAddressTaken(). Apologies, I should have handled this earlier. |
| 2185 ↗ | (On Diff #333088) | Well, the RemoveAttribute is indeed setting attributes by making call to setAttributes() and the utility function is just wrapping the commonly used code. Should we rename the function? |
| llvm/test/Analysis/CallGraph/ignore-bitcast-call-argument-callee.ll | ||
| 13 | Yeah, output is same. But the test case shows that hasAddressTaken() return value is indeed correct when there is a direct call as well as bitcast callees is used as an argument as is used by call graph builder. The similar tests in Analysis/CallGraph,check only the call graph. | |
| llvm/test/Transforms/GlobalOpt/bitcast-callees-fastcc.ll | ||
| 17 ↗ | (On Diff #333088) | This test demonstrates that the amendments made in this patch would actually set "fastcc" on bitcast callees. With trunk, "fastcc" would not be set on "foo" because hasAddressTaken() is true when queried by OptimizeFunctions() in GlobalOpt.cpp |
Comment Actions
The bot shows failure of bad-signature.c at https://reviews.llvm.org/B95560. When ran locally it does report the correct values and thus pass. I am not sure how to fix the test failure.
Failure in tile-and-distribute.mlir seems unrelated to this change.
@jdoerfert/ @arsenm / @rampitec , please advise.
Comment Actions
Thanks. This patch now addresses all known failures and thus I don't expect it to be a moving target from now.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2147 ↗ | (On Diff #333373) | This could be a function in a constant initializer for a global |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2147 ↗ | (On Diff #333373) | Right, that's what I mean. It could be anything, even a compare. |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2147 ↗ | (On Diff #333373) | Right, now I remember why I had dyn_cast and return in previous diff. |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2147 ↗ | (On Diff #333373) | Is there a test where the user is not a bit cast ? If not, please add one, similar for the other places that were changed back to dyn_cast |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2149 ↗ | (On Diff #333495) | You do not check that user is a callee. |
Comment Actions
I think this GlobalOpt patch is a separate change from the original one and needs to go first.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2149 ↗ | (On Diff #333495) |
And the original code does not check it either. This also warrants a test. |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2149 ↗ | (On Diff #333495) |
Done. I think we can introduce a utility function `CallBase* isCallBaseCallee(Use &U)` which can outline the below checks User *UU = U.getUser(); CallBase *CB = dyn_cast<CallBase>(UU); if (!CB) return nullptr; if (!CB->isCallee(&U)) return nullptr; return CB Not sure if this is an overkill. |
Comment Actions
This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2149 ↗ | (On Diff #333495) | You now have it in 3 different places, so it may make sense. It would look better if you combine conditions like '!CB || !CB->isCallee(&U))' and also 'isSomething' function shall return bool, not a pointer. |
Comment Actions
non call base users - What all such cases possible?We can use invoke but that is also CallBase.
non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.
Comment Actions
That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.
non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.
Yes, but this is not for global opt.
Comment Actions
Yes, but we should also consider about hasAddressTaken(). I am now starting to think if hasAddressTaken() needs to be more matured. Technically, you can cast a function pointer to global i8* and do all operations you suggest (e.g. use it as an operand in load) but hasAddressTaken() would return true in such cases because I am considering only CallBase users. If it needs to be more matured then we need to consider *all* instructions which take address as one of the operands.
non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.
Yes, but this is not for global opt.
Comment Actions
Not necessarily. We ignore some uses in hasAddressTaken() and that can leak to this place.
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
|---|---|---|
| 2147 ↗ | (On Diff #333373) | blockaddr-bitcast-fastcc.ll test address this. |
| llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll | ||
| 1 ↗ | (On Diff #333670) | This test case demonstates that fastcc is not set on function bar because its address taken value is true as hasAddressTaken() is used with its default param values. |
| llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll | ||
| 1 ↗ | (On Diff #333670) | Test case when bitcast is a call argument. |
| llvm/test/Transforms/GlobalOpt/blockaddr-bitcast-fastcc.ll | ||
| 1 ↗ | (On Diff #333670) | This test case demonstrates that fastcc is now set on function "g" unlike trunk which skips this function. This test also covers non call bases users case. |
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 | This does not respect IgnoreCallbackUses, IgnoreAssumeLikeCalls, IgnoreLLVMUsed. Also it does not handle bitcasts-of-bitcasts. Wouldn't the whole function be better implemented as a worklist algorithm now? SmallVector<Use &> WorkList(use_begin(), use_end());
while (!WorkList.empty()) {
U = WorkList.pop_back_val();
if (isa<BitCast>(U))
WorkList.push_back(U.use_begin(), U.use_end());
else if (isa<CallBase>(U))
... all the existing logic that handles Ignore* flags ...
} | |
Comment Actions
It looks fine to me with a style changes in couple places and positive checks in new tests.
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 | In general yes, but this patch is growing. This would be a good followup change. | |
| llvm/lib/Transforms/IPO/GlobalOpt.cpp | ||
| 2137 ↗ | (On Diff #333670) | A readbility change would be nice: if (!CB && !CB->isCallee(&U)) return |
| 2170 ↗ | (On Diff #333670) | Same here: if (!CB && !CB->isCallee(&U)) |
| llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll | ||
| 13 ↗ | (On Diff #333670) | You need to check something positive. You can just check it is 'define internal void @bar() {'. |
| llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll | ||
| 9 ↗ | (On Diff #333670) | Same here. |
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 | I disagree. You're changing a core API Function::hasAddressTaken in a way that could change the behaviour for all existing callers, whether they like it or not. So I think this part probably should be a separate patch and the implementation should be as rigorous as possible. | |
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 |
In general sense I do agree agree about the impact and that is why the first diff of this patch had an option to have the new code under an optional flag. Please have a look at the diff and the followup discussion on it.
I don't understand this part. Worklist based algorithm is more readble and maintenable but that is another way of implementing what this patch does and is not going to do anything different than this does functionality wise. If there is some functionality missing in this patch, then that's a different story. But I don't see that being a reason to block further progress on this patch. | |
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 |
The code you added to check callers-via-bitcasts does not respect the Ignore* flags, which could cause problems for existing users of this function. The worklist thing was just a suggestion for how to fix that (and also handle bitcasts-of-bitcasts-... in a natural way, though I'm not sure if they ever occur in practice). | |
| llvm/lib/IR/Function.cpp | ||
|---|---|---|
| 1630 | Sorry, I was confused. I see now that you're just adding yet more cases which don't count as "taking the address" of the function, which shouldn't cause any problems. I still think this function is getting too complicated and could do with refactoring. | |
| llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll | ||
|---|---|---|
| 13 ↗ | (On Diff #333670) | Added check for foo as well. |
Comment Actions
Seems reasonable now, haven't looked into the Attributor test changes but I expect unrelated side effects. I'm not blocking this anymore.
Comment Actions
This change broke the native build on x32 (x86_64 with 32-bit pointers) for me:
[46/3990] Building C object projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o /glaubitz/llvm-project/stage1.install/bin/clang -DVISIBILITY_HIDDEN -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/glaubitz/llvm-project/compiler-rt/lib/builtins -Iinclude -I/glaubitz/llvm-project/llvm/include -fPIC -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter -O3 -m64 -fno-lto -std=c11 -UNDEBUG -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -c /glaubitz/llvm-project/compiler-rt/lib/builtins/cpu_model.c clang: /glaubitz/llvm-project/llvm/include/llvm/Support/Casting.h:269: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::CallBase; Y = llvm::User; typename llvm::cast_retty<X, Y*>::ret_type = llvm::CallBase*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump: 0. Program arguments: /glaubitz/llvm-project/stage1.install/bin/clang -DVISIBILITY_HIDDEN -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/glaubitz/llvm-project/compiler-rt/lib/builtins -Iinclude -I/glaubitz/llvm-project/llvm/include -fPIC -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter -O3 -m64 -fno-lto -std=c11 -UNDEBUG -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -c /glaubitz/llvm-project/compiler-rt/lib/builtins/cpu_model.c 1. <eof> parser at end of file 2. Optimizer #0 0x01b23df2 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1b23df2) #1 0x01b21e10 llvm::sys::RunSignalHandlers() (/glaubitz/llvm-project/stage1.install/bin/clang+0x1b21e10) #2 0x01a8ea17 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0 #3 0xfffffffff7f021e0 __restore_rt (/lib/x86_64-linux-gnux32/libpthread.so.0+0x131e0) #4 0xfffffffff7a2886d raise (/lib/x86_64-linux-gnux32/libc.so.6+0x3286d) #5 0xfffffffff7a12515 abort (/lib/x86_64-linux-gnux32/libc.so.6+0x1c515) #6 0xfffffffff7a123fe (/lib/x86_64-linux-gnux32/libc.so.6+0x1c3fe) #7 0xfffffffff7a211ff (/lib/x86_64-linux-gnux32/libc.so.6+0x2b1ff) #8 0x019f65c0 llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (/glaubitz/llvm-project/stage1.install/bin/clang+0x19f65c0) #9 0x015d3b7d llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x15d3b7d) #10 0x02ae78dd llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x2ae78dd) #11 0x0146f513 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x146f513) #12 0x01de06a1 (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (.constprop.0) BackendUtil.cpp:0:0 #13 0x01de3ae2 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1de3ae2) #14 0x02a844dd clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x2a844dd) #15 0x0368be90 clang::ParseAST(clang::Sema&, bool, bool) (/glaubitz/llvm-project/stage1.install/bin/clang+0x368be90) #16 0x02424b3c clang::FrontendAction::Execute() (/glaubitz/llvm-project/stage1.install/bin/clang+0x2424b3c) #17 0x023bb424 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x23bb424) #18 0x024e0fd3 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/glaubitz/llvm-project/stage1.install/bin/clang+0x24e0fd3) #19 0x008dd673 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/glaubitz/llvm-project/stage1.install/bin/clang+0x8dd673) #20 0x008d921d ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) driver.cpp:0:0 #21 0x02262c99 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const::'lambda'()>(int) Job.cpp:0:0 #22 0x01a8eb8d llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1a8eb8d) #23 0x02263ecb clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (.part.0) Job.cpp:0:0 #24 0x0223911c clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/glaubitz/llvm-project/stage1.install/bin/clang+0x223911c) #25 0x02239b37 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/glaubitz/llvm-project/stage1.install/bin/clang+0x2239b37) #26 0x022452f4 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x22452f4) #27 0x0085ecea main (/glaubitz/llvm-project/stage1.install/bin/clang+0x85ecea) #28 0xfffffffff7a13da2 __libc_start_main (/lib/x86_64-linux-gnux32/libc.so.6+0x1dda2) #29 0x008d8bbb _start (/glaubitz/llvm-project/stage1.install/bin/clang+0x8d8bbb) clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation) clang version 13.0.0 Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /glaubitz/llvm-project/stage1.install/bin clang-13: note: diagnostic msg: ******************** PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT: Preprocessed source(s) and associated run script(s) are located at: clang-13: note: diagnostic msg: /tmp/cpu_model-3ac5e8.c clang-13: note: diagnostic msg: /tmp/cpu_model-3ac5e8.sh clang-13: note: diagnostic msg: ********************
I'll file a bug report.
Comment Actions
@madhur13490 This is causing some build failures: http://lab.llvm.org:8011/#/builders/121/builds/6287
Comment Actions
@glaubitz Now that there's a repro - please could you revert this so @madhur13490 can investigate it offline?
Comment Actions
I have an auto-bisecting cron job that has identified this change as a second-stage regression on Fedora 34 (x86-64). Basically llvm-tblgen crashes reliably:
FAILED: lib/Target/X86/X86GenDAGISel.inc cd /tmp/_update_lc/t && /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump: 0. Program arguments: /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d #0 0x000000000050b153 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/bin/llvm-tblgen+0x50b153) #1 0x00000000005094b2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/llvm-tblgen+0x5094b2) #2 0x000000000050b4fa SignalHandler(int) Signals.cpp:0:0 #3 0x00007ffff7fa6a00 __restore_rt sigaction.c:0:0 #4 0x0000000000316a7d (/tmp/_update_lc/t/bin/llvm-tblgen+0x316a7d)
Is this issue known?
Comment Actions
We've also seen failures on all of our 2 stage bots e.g. http://lab.llvm.org:8011/#/builders/7/builds/2348. Which are fixed now this is reverted.
[494/5842] Building Attributes.inc... FAILED: include/llvm/IR/Attributes.inc cd /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2 && /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/bin/llvm-tblgen -gen-attrs -I /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/include -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR/Attributes.td --write-if-changed -o include/llvm/IR/Attributes.inc -d include/llvm/IR/Attributes.inc.d pure virtual method called terminate called without an active exception
Bisecting this I have seen clang-tblgen crash with a few different asserts. Bad alloc, one in erase() in StringMap, illegal instruction. Our bot doesn't have sanitizers on so it could be the same finicky memory corruption.
This PPC one does though https://lab.llvm.org/buildbot/#/builders/18/builds/1140 and saw similar failures. No immediate explanation but some configs worth checking locally before the reland.
Comment Actions
Back to green after the revert: https://lab.llvm.org/buildbot/#/builders/121/builds/6307
Comment Actions
I think it was also causing this failure in my Release+Asserts build:
/home/jayfoad2/git/llvm-project/llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll:150:16: error: CHECK-LABEL: expected string not found in input
; CHECK-LABEL: define internal void @branch_funnel(i8*
^
<stdin>:81:16: note: scanning from here
define i32 @fn3(i8* nocapture readonly %obj) local_unnamed_addr #2 {
^
<stdin>:95:18: note: possible intended match here
define hidden void @__typeid_typeid1_0_branch_funnel(i8* nest %0, ...) local_unnamed_addr #6 {
^
...
Failed Tests (1):
LLVM :: Transforms/WholeProgramDevirt/branch-funnel.llComment Actions
These failures were not caught by LLVM bots which run on a patch. Can I do a trial run of the bots and run all tests that would run when the patch lands?
Comment Actions
Apparently my change exposes the memory corruption bug on X86 too. As the bots show, I can't generate .inc for almost all ARM .td files. Not sure how to fix this reliably. Surprisingly, the builds pass in Debug build type.
Comment Actions
This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.
Debugging:
- Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
- Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
- Running valgrind also did not reveal any new things.
- Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.
However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.
Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.
Comment Actions
This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?
Comment Actions
There is no failing IR but the failing binary. Tablegen constitutes around ~200 C/C++ files and all I know is that the binary is misompiling. It is pretty difficult to say at this point if one single file is miscompiling or a bunch of them. All I know at this point is
- A lot of new functions are now added to IPSCCP's radar as hasAddressTaken() for them is 'false' now as it meets the condition my patch adds i.e all users of bitcast operator are CB. See llvm::canTrackArgumentsInterprocedurally(Function *F) which is called from IPSCCP.
- IPSCCP may not be the culprit but it is definitely a starting point of my investigation. There are obvious places in it which needs fix e.g. properly removing argmemonly and inaccessiblemem_or_argmemonly attributes here.
Comment Actions
Are we sure this is not a single bad cast that evolves into something different if assertions are disabled. Is the cast problem located, that should be easy after all given the minimal reproducer in PR49861?
Comment Actions
The two line reproducer points to one part of the problem and it does fix cpu_model.c assert (mentioned in PR49861) but it does not help the miscompilation of TableGen. All my experiments are done after I fix the problem pointed by assert in cpu_model.c.
Comment Actions
You can use a Debug Counter and bisect it to the first function for which the hasAddressTaken change will cause the issue.
With the pre- and post-IPSCCP module we should be able to figure this out.
Done by lint.