In D92486#2428865, @fhahn wrote:

In D92486#2428821, @nikic wrote:

Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=54eab293f523956bdc4b1a98b6cf5abc0bd1ef3f&to=b023c8adc9eb545ec5b8aa1a8d6f77a46d622006&stat=instructions

The problem here is that LoopSink is only relevant for PGO, but MemorySSA gets computed unconditionally in the legacy PM. This problem does not affect the new pass manager, where it's possible to compute MemorySSA only if useful. Supporting this properly in LegacyPM would require something like LazyMemorySSA.

I think in this instance it might be best to do this change only for NewPM. It's best to keep both pass managers in sync, but here there is a technical reason for different treatment.

Perhaps it would also be possible to directly construct MemorySSA in the pass, if PGO is enabled with the legacy PM?

In D92401#2427794, @jdoerfert wrote:

In D92401#2426503, @nikic wrote:

In D92401#2426402, @jdoerfert wrote:

This kind of logic should always be valid, regardless if V1 is a GEP or not, right? Is there a way to do this check early or late for any query?

The general idea is valid, but the way it is applied depends on the operation. For GEPs we just strip to the base pointer. For phis and selects, we recurse over the phi/select operands (with special casing for the case of identical control dependence).

Hm, I'm confused(, but also not deep enough in the BasicAA code to argue). I assumed that this new logic which is put in a function that has a precondition (isa<GEP>(V1)) could be placed in a function without that precondition. The logic does not utilize the fact that V1 isa GEP. Anyway, it was just a thought.

The problem here is that LoopSink is only relevant for PGO, but MemorySSA gets computed unconditionally in the legacy PM. This problem does not affect the new pass manager, where it's possible to compute MemorySSA only if useful. Supporting this properly in LegacyPM would require something like LazyMemorySSA.

It looks like this change has also enabled the new pass manager by default.

In D92401#2426402, @jdoerfert wrote:

This kind of logic should always be valid, regardless if V1 is a GEP or not, right? Is there a way to do this check early or late for any query?

I don't think this change makes a lot of sense. I expect that in nearly all cases we'll be able to produce a better result by having a separate PoisonValue check beforehand (we'll want to return poison rather than undef or some other value). And if we do that, this change to the isUndefValue() API just makes things more confusing.

LGTM

@asbirlea Is this ready to land in the current form?

LGTM

LGTM

This will clearly need something more sophisticated. After thinking this over, I also think that the current approach of inserting assumptions at phis is not optimal, as the tests added in https://github.com/llvm/llvm-project/commit/b5e8de9c7903d458b098a8af03939384270c1a5e show. I expect this can also result in BatchAA returning different results, depending on whether a gep or a phi is visited first. We should be inserting everything as NoAlias into the cache in the first place and tracking whether that assumption gets used or not.

There seem to be still quite a few places where UndefValue can be replaced with PoisonValue in ConstantFold, do you plan to follow up on that?

@mstorsjo Thanks for the report & revert.

Rebase

LGTM

Looks like this change has a large negative compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=2254e014a9019bf17c3f5cb27c1dc40ca0f2ffce&to=14f2ad0e3cc54d5eb254b545a469e8ffdb62b119&stat=instructions 2% on average, 6% on mafft.

Unless there's something that makes this issue specific to the call slot optimization, wouldn't it be better to adjust the logic in MDNode::getMostGenericAliasScope() to ensure that any code performing alias scope merging is covered?

The logic here looks sound to me. Structurally, I would suggest integrating this with the getPointerDereferenceableBytes() check above, as you are currently duplicating the logic around it. Basically extract the current getPointerDereferenceableBytes() call into a static function/lambda, and add the handling for the allocated object size in there, the remaining logic (including the non-null check and the alignment check) will be shared.

LGTM

Rebase. I've committed the not-really-related change that lead to the compile-time improvement. With that gone, the new numbers are: http://localhost:8000/compare.php?from=6f5ef648a57aed3274118dcbd6467e8ac4f6ed1f&to=50d73a31620111c4298cc336ac4682ef7585ed6b&stat=instructions Which does make this a mild compile-time regression, but not too bad.

In D90648#2409760, @davidxl wrote:

This patch does not have a new test case added to demonstrate what this patch is trying to fix or reference the bug number showing the bug end-to-end.

LGTM

In D89207#2370224, @asbirlea wrote:

Since this is using MSSA walker API with a MemoryLocation (which does not cache), could this become costly in a pathologically constructed example? If so, could we have a (reasonably large) upper bound on the number of getClobbering (Access, Location) calls to avoid such cases?

Rebase.

Rebase over D91649, after which this correctness of this change should be more obvious. At this point in BasicAA, all LocationSizes only allow access after the base pointer.

Accept AATags in MemoryLocation::getAfter/getBeforeOrAfter helpers, which makes them usable in more cases.

Rename the MemoryLocation helpers as well. With the new LocationSize naming, it makes sense to call these MemoryLocation::getAfter() and MemoryLocation::getBeforeOrAfter().

Rename to LocationSize::afterPointer() and LocationSize::beforeOrAfterPointer().

Rebase, address review.

I'm not sure. It might make more sense to make SimplifyCFG be less aggressive about forming switches in this case. Pretty sure that converting if (a == C || B == C2) into a switch is pretty much a strict regression in optimization power, because a lot more passes support branches than switches.

Looking over the patch, the compile-time regression is likely not related to MemorySSA, but the fact that you moved the AST construction before the !Preheader->getParent()->hasProfileData() check, and the profitability heuristic. This means that expensive AST construction now happens every time, instead of only when useful.

This change showed up as a significant compile-time regression (before it was reverted): https://llvm-compile-time-tracker.com/compare.php?from=85ccdcaa502ee2c478f2d0ba2b1e217117b69032&to=d4ba28bddc89a14885218b9eaa4fbf6654c2a5bd&stat=instructions

@uabelho Thanks for the report! I have reverted the assert in https://github.com/llvm/llvm-project/commit/85ccdcaa502ee2c478f2d0ba2b1e217117b69032. We'll have to fix this asymmetric in phi-phi aliasing before re-enabling it.

In D91576#2401162, @hoy wrote:

In D91649#2401252, @asbirlea wrote:

• LocationSize::unknownMaybeNegative() seems somewhat ambiguous in isolation; unless I see the other name or have the context from this patch. I'm either keep LocationSize::unknown or make it even more verbose LocationSize::unknownPositiveOrNegative().

Can't really comment on whether this makes sense approach wise. For testing, I'd suggest to test that this is correctly handled in bitcode serialization/loading as well, and to drop the current hack in PredicateInfo and see if it works fine for that use case.

The approach here doesn't look right to me. Poison flags have no relation to the position where a certain instruction is executed. What's relevant is where the instruction is used.

I don't get this fix. Adding extra scopes to alias.scope should always be conservatively correct. I think there may be some confusion here due to the malformed noalias metadata you're using.

Strip pointer casts in GlobalsModRef. This *might* be the cause for observed differences, as previously the call from BasicAA would have been performed with casts stripped.

While I believe this change is correct, there are some incorrect AA uses that should be addressed first. There's one in MemorySSA wrt phi translation over decreasing cycles (https://reviews.llvm.org/D87778#inline-852179), and another in ParallelDSP (see test failure).