This is an archive of the discontinued LLVM Phabricator instance.

[IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction
ClosedPublic

Authored by madhur13490 on Mar 18 2021, 12:14 PM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2021, 12:14 PM
madhur13490 added inline comments.Mar 18 2021, 12:16 PM
llvm/include/llvm/IR/Function.h
879 ↗(On Diff #331651)

Done by lint.

llvm/lib/IR/Function.cpp
1615

Done by lint

What problem are you trying to solve?

rampitec requested changes to this revision.Mar 18 2021, 12:33 PM
rampitec added inline comments.
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.

This revision now requires changes to proceed.Mar 18 2021, 12:33 PM
rampitec added inline comments.Mar 18 2021, 12:34 PM
llvm/test/Analysis/CallGraph/ignore-bitcast-callees.ll
12

remove #1, here and below.

arsenm requested changes to this revision.Mar 18 2021, 12:52 PM

Users can just call stripPointerCasts first.No need to add a special case here(which is also incoplete)

This test also does not really work until you add true argument to the call graph builder.

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

madhur13490 added a comment.EditedMar 19 2021, 3:48 AM

What problem are you trying to solve?

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.

address review comments

madhur13490 marked 2 inline comments as done.Mar 19 2021, 11:57 AM

This test also does not really work until you add true argument to the call graph builder.

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?

remove unnecessary changes

madhur13490 marked an inline comment as done.Mar 19 2021, 12:00 PM

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.

This test also does not really work until you add true argument to the call graph builder.

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?

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.

rebase, address review comments to remove the optiona flag, update summary, rename and fix tests.

revert unnecessary changes to comments.

rampitec added inline comments.Mar 22 2021, 11:29 AM
llvm/include/llvm/IR/Function.h
887 ↗(On Diff #332208)

Revert this change, you are not touching this file anymore.

llvm/test/Analysis/CallGraph/ignore-bitcast-call-argument-callee.ll
13

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
17

Can you add a second identical call to the same bitcast? So that you have test where the same bitcast is used twice.

rebase, remove function.h, add one more test

madhur13490 marked 2 inline comments as done.Mar 23 2021, 12:54 AM
rampitec accepted this revision.Mar 23 2021, 8:30 AM

LGTM, thanks!

Commit message needs update

madhur13490 added a comment.EditedMar 23 2021, 10:08 AM

Commit message needs update

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/

@rampitec, @jdoerfert @arsenm

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?

@rampitec, @jdoerfert @arsenm

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?

Maybe just strip bitcasts there?

@rampitec, @jdoerfert @arsenm

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.

No, don't add cruft due to FUD

@rampitec, @jdoerfert @arsenm

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.

No, don't add cruft due to FUD

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.

fix failing tests reported by bot, handle globalopt when bitcast callees present

@rampitec, @jdoerfert @arsenm

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.

No, don't add cruft due to FUD

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.

Okay, please review further changes.

madhur13490 retitled this revision from [IR] Add opt-in parameter to hasAddressTaken() to ignore bitcasts callee in callbase instruction to [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction.Mar 24 2021, 2:07 AM
madhur13490 edited the summary of this revision. (Show Details)

Commit message needs update

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/

Updated summary manually on phab.

jdoerfert added inline comments.Mar 24 2021, 8:53 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2160

Now we could even have a helper of some sort. Unsure what design is best.

llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
6

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

Same as above but I do not understand why this change would cause us to loose the function attributes if no AA is provided.

use script to update the test, add utility function and address other parts of global opt to fix clang tests

madhur13490 added inline comments.Mar 24 2021, 12:29 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2160

Sounds fine. Outlined in a separate function.

llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
6

Done now, Didn't know the script is so powerful!

arsenm added inline comments.Mar 25 2021, 10:36 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2135–2139

Just use cast<>

2169–2173

Just use cast<>

madhur13490 added inline comments.Mar 25 2021, 10:43 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2169–2173

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?

arsenm added inline comments.Mar 25 2021, 10:45 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2169–2173

cast<> asserts in a debug build. dyn_cast is when you are trying to handle the null case

@jdoerfert This patch is blocking https://reviews.llvm.org/D99347 for AMDGPU. Please have a look.

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

This can at some point error out. Why would the user of ChangeCalleesToFastCall and others need to require this property?

2158

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?

2188

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
18

Also unclear what is tested here? Looks like the trunk output.

use cast<> instead of dyn_cast<>

rampitec requested changes to this revision.Mar 25 2021, 11:38 AM

Since it keeps changing revoking the accept vote for now.

This revision now requires changes to proceed.Mar 25 2021, 11:38 AM
madhur13490 added inline comments.Mar 25 2021, 12:02 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2135–2139

I think I am missing something here. This function is capturing common logic used by ChangeCalleesToFastCall.

2158

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.

2188

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
18

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

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.

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.

The bots are imperfect, I would say it's unrelated

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.

The bots are imperfect, I would say it's unrelated

Thanks. This patch now addresses all known failures and thus I don't expect it to be a moving target from now.

rampitec added inline comments.Mar 25 2021, 1:14 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

I do not see why a user of bitcast must be necessarily a call.

2185

Same here, it is not necessarily a call.

arsenm added inline comments.Mar 25 2021, 1:19 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

This could be a function in a constant initializer for a global

rampitec added inline comments.Mar 25 2021, 1:24 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

Right, that's what I mean. It could be anything, even a compare.

go back to dyn_cast to handle all users

madhur13490 added inline comments.Mar 25 2021, 9:41 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

Right, now I remember why I had dyn_cast and return in previous diff.

fhahn added a subscriber: fhahn.Mar 26 2021, 1:52 AM
fhahn added inline comments.
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

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

rampitec added inline comments.Mar 26 2021, 9:39 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

You do not check that user is a callee.

I think this GlobalOpt patch is a separate change from the original one and needs to go first.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

You do not check that user is a callee.

And the original code does not check it either. This also warrants a test.

specifically check for callee

madhur13490 added inline comments.Mar 26 2021, 10:26 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

You do not check that user is a callee.

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.

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.

rampitec added inline comments.Mar 26 2021, 10:31 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

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.

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.

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.

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.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

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.

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.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.

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.

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.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.

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.

Not necessarily. We ignore some uses in hasAddressTaken() and that can leak to this place.

add new test

madhur13490 added inline comments.Mar 27 2021, 6:32 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2153

blockaddr-bitcast-fastcc.ll test address this.

llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
1

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

Test case when bitcast is a call argument.

llvm/test/Transforms/GlobalOpt/blockaddr-bitcast-fastcc.ll
1

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.

foad added a subscriber: foad.Mar 29 2021, 8:30 AM
foad added inline comments.
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 ...
}

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.
The pass calls hasAddressTaken() without arguments so that is not exerciseable here as is. For the same reason I withdraw my request to split the patch. Also note this may change if this is landed: D96179.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
2137

A readbility change would be nice:

if (!CB && !CB->isCallee(&U))
  return
2170

Same here: if (!CB && !CB->isCallee(&U))

llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
13

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

Same here.

foad added inline comments.Mar 30 2021, 2:15 AM
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.

madhur13490 added inline comments.Mar 30 2021, 3:30 AM
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.

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.

So I think this part probably should be a separate patch and the implementation should be as rigorous as possible.

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.

foad added inline comments.Mar 30 2021, 3:38 AM
llvm/lib/IR/Function.cpp
1630

If there is some functionality missing in this patch, then that's a different story.

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).

foad added inline comments.Mar 30 2021, 7:05 AM
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.

Addressed Stas' comments

madhur13490 marked an inline comment as done.Mar 30 2021, 7:25 AM
madhur13490 added inline comments.
llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
13

Added check for foo as well.

rampitec accepted this revision.Mar 30 2021, 9:31 AM

LGTM with a nit in 2 tests. Please also wait for other responses.

llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll
11

Make a positive check for foo.

22

Also need positive check for test.

madhur13490 marked an inline comment as done.Apr 3 2021, 8:13 PM

Ping!

jdoerfert resigned from this revision.Apr 5 2021, 10:21 AM

Seems reasonable now, haven't looked into the Attributor test changes but I expect unrelated side effects. I'm not blocking this anymore.

make positive checks

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2021, 2:24 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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.

@glaubitz Now that there's a repro - please could you revert this so @madhur13490 can investigate it offline?

@glaubitz Now that there's a repro - please could you revert this so @madhur13490 can investigate it offline?

I don't have commit access, unfortunately.

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?

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.

foad added a comment.Apr 6 2021, 8:18 AM

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.ll

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?

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?

I don’t think LLVM is set up for that. Can you try two stage testing first?

madhur13490 added a comment.EditedApr 8 2021, 2:09 AM

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.

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.

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:

  1. 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.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. 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.

arsenm added a comment.Apr 9 2021, 1:11 PM

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:

  1. 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.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. 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.

This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

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:

  1. 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.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. 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.

This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

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

  1. 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.
  2. 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.

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?

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?

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.

  1. 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.

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.