I have been investigating the behavior of struct path tbaa and new struct path tbaa. Based on what I could find, returning MD is the correct thing to do in struct path tbaa.

@wsmoses I will not have a solution today, but hopefully tomorrow. I would appreciate if you could help review then. Thanks.

FYI: I am looking into an alternative fix.

In D99851#2678971, @aheejin wrote:

@jeroen.dobbelaere Sorry not sure if I understand. Are you planning to review the code?

Please do not commit this yet. A ping would have been appreciated.

In D90098#2673996, @jeroen.dobbelaere wrote:

In D90098#2673969, @nikic wrote:

@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).

Yes, that might be a valid path.

On the other hand, a ptr_provenance path skips pointer computations. We might need to refactor this code to separate the 'based on' part, so we can first check the ptr_provenance path (if available), and when that is not conclusive,
we can fall back to the original pointers (that also contain GEP information etc).

In D90098#2673969, @nikic wrote:

@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).

I am wondering how this will interact with the ptr_provenance changes/full restrict changes. In the full restrict version, V1AAInfo and V2AAInfo are used in BasicAAResult::aliasCheck to decide if we can see trough noalias intrinsics or not.

ping

• Deprecating LLVMIntrinsicCopyOverloadedName
• Added release note.

Dropping in favor of D99173.

• rebased
• Introduced LLVMIntrinscicCopyOverloadedName2(...), Introduced `getBaseName(...)'

In D99066#2641561, @fhahn wrote:

How many places would need to be updated? It sounds like there's no nice solution (automatically catch cases that really need the module), but it also seems brittle to have Intrinsic::getName crash in various places because of missing Module arguments.

In D99066#2641043, @fhahn wrote:

Is there a way to ensure all call sites that may need to handle opaque pointers pass the module? can we add an assertion ensuring the module is passed if the intrinsic takes overloaded pointer types?

In D91250#2639839, @fhahn wrote:

You should be able to download the reproducer. Running LLVM-dis should crash. https://oss-fuzz.com/download?testcase_id=5766959267446784

In D91250#2639731, @fhahn wrote:

It looks like this causes assertion failures when dis-assembling some bitcode https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32288

LGTM

Thanks for taking the initiative !

Rebased. Moved ~PredicateInfo() to header.

Rebased. Modified langref and test according to comments.

Thank you for all the help with the review !

Fixed an unwanted reversal of instructions shown in the testandor.ll test.

The pr48340.ll test was also moved to the X86 directory.

Changed according to comments.

• rebased

• added reference to and testcase for PR48340
• rebased

This is an attempt to trigger the discussion. This patch fixes the observed issue, but it might not be the right place to do ?

I am now tempted to think that any same-scope usage of llvm.experimental.noalias.scope should be avoided..
For something like loop-unswitch, it is not 'wrong' conceptually to have the same scope, but always cloning the scope
should be safer, with, in most cases, no impact on code quality.

Thanks for taking care of this !

In D96949#2571994, @nikic wrote:

Do you have any particular examples? I would expect that most builtins get lowered to intrinsics and those should already have all the necessary attributes, but there's probably some cases that fell through.

In D96949#2571873, @nikic wrote:

After D96960 I don't think this change is really necessary anymore (at least it does not need an LLVM 12 backport). If the language has forward progress, then all function definitions will get annotated anyway, so only extern declarations are relevant. However, those will not be readyonly either -- unless they are explicitly annotated such, in which case they will also be annotated as willreturn after D96960.

Any chance an updated version of this improvement can be added to bugpoint ?

At https://llvm-compile-time-tracker.com/index.php?config=O3&stat=instructions&branch=dobbelaj-snps/perf/InlineFunction_same_inst_20210202_01 there is a comparison of two possible approaches.
But I do not have the impression that it makes a big difference for these testcases.

In D95544#2533355, @nikic wrote:

@jeroen.dobbelaere Can you please also re-enable the verifier flag (assuming you're not aware of further issues right now)?

Note for those that have not been following the LLVM AA Technical Calls: we have introduced part of the infrastructure needed for full restrict by focusing on fixing https://bugs.llvm.org/show_bug.cgi?id=39282 (See D93039, D93040, D92887, D94306).

Adapted to comments.

In D95335#2522370, @jeroen.dobbelaere wrote:

There seems to be a problem with bad duplication in the MemorySanitizerLegacyPass.

There seems to be a problem with bad duplication in the MemorySanitizerLegacyPass.

I disabled the dominance check by default in rG3b5d36ece21f. Several buildbots show examples where this triggers, indicating there are still passes not taking care of proper handling the noalias scopes.

Adapted to comments.

For a stage2 release build and the test-suite, the largest amount of noalias scope decls for a function is 1075 (triggered by llvm/unittests/ADT/SmallVectorTest.cpp). Everything else is <1000. The largest amount of same scope declarations seen was 5.

For what it's worth, the number '1000' comes from experiments triggering the worst case behavior. With 1000 basic blocks and scope declarations, the time for parsing and checking on my testmachine was roughly 0.1 seconds. (Going to approx 1 second for 4000 blocks and declarations).

Will you do the flip or shall I do it ?

Small refactoring as recommended by @jdoerfert

Thanks. I'll still do the small refactoring recommended by @jdoerfert and then do the commits.

Use NoAliasScopeDeclInst. Only analyze non-debug instructions with metadata. (*1)