In D58091#1396437, @aaron.ballman wrote:

In D58091#1396397, @jdoerfert wrote:

In D58091#1396382, @aaron.ballman wrote:

• but I wonder why those diagnostics are happening in the first place. It seems like the warning is still useful when it triggers outside of that situation, no?

The underlying conceptual problem, which I didn't know when I added GE_Missing_type, is that this has _nothing_ to do with the location of the declaration. We say, include the header X.h, if we were not able to build a type for recognized built-in Y that should be declared in X.h. However, we should report _why_ we could not build the type instead. For built-ins we do not have a type on record (GE_Missing_type), this is always, so no warning for now. For the ones that we only fail to build a type because some requirement is missing, we should report that, at least when we are in the respective header. I don't have a perfect solution of what to do actually.

I could check if the declaration is (probably) in the respective header so we can switch between warnings?

That's kind of what I was wondering, but I deal with builtins so infrequently that my expectations may be wrong here. If a user declares a builtin with a conflicting type outside of a header file, that seems like we'd want to warn the user about right? But this seems to remove that warning, at least in the case of test/Sema/implicit-builtin-decl.c:71. Or do I misunderstand the situation causing the warning to trigger?

In D58091#1396393, @bcain wrote:

I reported PR40692. I just tried this patch on our local build where we saw the failure on an RTOS implementing pthreads. Unfortunately with this patch I encountered an (unrelated) assertion. So this fix was inconclusive for me (for now). I will follow up but if this fix makes sense then you don't need to wait for my test results.

In D58091#1396382, @aaron.ballman wrote:

If I'm following along properly, it sounds like we want to disable this warning largely because it can appear in header files attempting to declare the functions in question.

In D57052#1367472, @bjope wrote:

In D57052#1366530, @jdoerfert wrote:

I see your point and I acknowledge it is a problem. However, I'm not sure a check in IPCP and consequently later in other IPOs is the best solution.
While I have some ideas on how to deal with this (one is to not create call sites for this situation), I think we can go ahead with your local fix now,
keep the test case and make it more general as needed.

Long story short, LGTM.

One thing, can you mention that this problem was exposed by the fact that AbstractCallSites will look through the bitcast at the callee position of a call/invoke somewhere?

Thanks, I added that info to the summary. Now waiting to get my commit access back (seems like our legal department haven't signed the new license agreement yet... so this can't be landed yet... any day now...).

I see your point and I acknowledge it is a problem. However, I'm not sure a check in IPCP and consequently later in other IPOs is the best solution.
While I have some ideas on how to deal with this (one is to not create call sites for this situation), I think we can go ahead with your local fix now,
keep the test case and make it more general as needed.

Initial comments from my side.

In rL351629#307287, @RKSimon wrote:

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/23003/steps/build/logs/warnings%20%284%29

Does the new documentation make sense?

Fix wording

Improved the documentation

Generalize the treatment of "kw_this" as "kw_ident"

Small fixes, allow "this" inside "callbacks"

Update to the correct commit again.

Remove leftover debug stmt

Updated according to your comments.

Style changes, clang-format, documentation improvements

Allow to use names in the callback metadata

Fix tests

Update encoding, rebase to HEAD

Rebase to HEAD

Rebase to HEAD and test fixes

Update the old encoding

Removed IPConstProp, improved documentation/wording and metadata encoding

In D54498#1339322, @hfinkel wrote:

A high level strategy comment. I think you're trying to go a bit too far in one patch. I'd advise first writing a patch which does nothing but implement the IPConstProp adjustments "the hard way" (i.e. without the AbstractCallSite) and focus on defining the metadata with the semantics you once. To really justify the complexity of the abstraction, you need more than one user. You could add another user to this patch, but doing so will just slow down the review. Separating this into a series of patches (IPConstProp w/metadata def, second user, then introduce abstraction) will make it much easier to see the necessity and make each piece more easily reviewable.

Splitting the patch and providing multiple use cases does not necessarily require rewriting the IPConstProp without the abstraction. This can be split into multiple (dependent) patches and other dependent patches can also be posted showing other use cases.

Improve LangRef wording

Check for isThreadDependent constants

In D54498#1327869, @efriedma wrote:

Does the ipcp transform need to check isThreadDependent() on the Constant?

Thanks for the feedback.

Fix and improve documentation

Update according to initial feedback

Update to new encoding

In D55483#1326029, @aaron.ballman wrote:

This is missing all of the Sema and SemaCXX tests. Should have tests for member functions, variadic functions, incorrect arguments, incorrect subjects, etc.

Remove accidental changes

Use metadata encoding, fix spelling and other minor mistakes

Removed s/zext, moved ptr2int to make sense. Bitcast was my actual use case, int2ptr was not (always) covered before, the ptr2int test case was but only late in computeKnownBits.

Code movement and removal (based on the comments by Eli Friedman)

Use auto * and remove obsolete modification

Ensure no-op or extension casts and add appropriate tests. Also fix an error in LazyValueInfo which looked through AddrSpaceCast to determine non-nullness.

I'm also running the LLVM-TS to check nothing breaks.

Look through even more instructions

In D54956#1309964, @fhahn wrote:

Would it be possible to add a test case?

Add impacted test cases

Reverting for now as there are active miscompiles caused by the current version.

In D53342#1306336, @xbolva00 wrote:

And we would be even more powerful with FunctionAttrs as a Module pass to use DT in arg attribute promotion logic (now we do it only for entry block).

Some comments and questions.

In D54498#1305504, @fhahn wrote:

FWIW TransitiveCallSite seems like a slightly better name to me than AbstractCallSite, as the CallSite class is already an abstraction over different kinds of calls. As for the location of the class, if it is tied to an analysis that gathers the transitive call sites, having it in lib/Analysis/ might make sense initially as well.

In D54498#1298768, @bollu wrote:

Thanks for the answer! That clears up a lot.

So, I believe the main difference between the pthread example and the one that I have is that of mutual recursion: I will often have mutual recursion between the trampoline and the f_i, which usually complicates inter-procedural analysis (as far as I understand?)

In D54498#1298715, @bollu wrote:

I have a question in the context of optimising sequential programs -- I'm interested in optimising across a trampoline call, since I have a haskell-like lazy language, which lowers CPS into LLVM-IR by constructing a trampoline. Example IR here

Would the AbstractCallSite representation allow one to represent the control flow between:

trampoline -> fn1 -> trampoline -> fn2 -> ...

Thanks for the comments!

Thanks for the patch!