Addressing comments.

Created new review instead of update. Sorry for noise.

Updated threshold and conditional branch cost-size after performance testing.

@nikic, I didn't notice your comments before push, will address your comments in different patch.

Removed unneeded change in MemorySSATest.cpp.

Rebased after preparation changes (D98027+D98718) committed.

Rebased to use reworked AliasResult in D98718.

I was not sure this patch will be landed so left GVN patch based on previous already landed (cached) offset storage.

@nikic, @asbirlea would you please review the patch?
It blocks GVN related D95543.

Ping...

In D98718#2635171, @nikic wrote:

I do see some impact from this patch: https://llvm-compile-time-tracker.com/compare.php?from=be92dcb9f679a23329d360bbc5533acc5f808d9c&to=1f10b8ef5f0243ac0a2679d6f334275471bff316&stat=instructions

In D98718#2635171, @nikic wrote:

I do see some impact from this patch: https://llvm-compile-time-tracker.com/compare.php?from=be92dcb9f679a23329d360bbc5533acc5f808d9c&to=1f10b8ef5f0243ac0a2679d6f334275471bff316&stat=instructions

I see there you experimented with 8 bits for Result and zero init for Offset field - I agree it's a good ideas.

• Fixed swapping already cached result.
• Result bit field size set to 8 bits, Offset field initialized with zero.

@nikic thanks for review, and what do you think about Optional<AliasResult> - should it be refactored to Optional<AliasResult::Result> in MemorySSA?
Or, possibly, will this module use PartialAlias offsets?

Addressed comments.

The patch should be updated after converting AliasResult.

Thanks!

Do you have any test for the fix?

Converted all AliasAnalysis::alias() == AliasResult::NoAlias to
AliasAnalysis::isNoAlias().

Made the enum unscoped back since it going to be unscoped within a class planned
to be new AliasResult.

No, I don't have a patch at the moment, but will upload one since you're generally ok with this step.

Implementation.

In D97676#2594791, @fvrmatteo wrote:

I noticed @dfukalov planned to fix the DSE after their patch, so I'm not sure if I should delete mine, as their fixes/improvements may be covering more cases compared to my isolated test file.

I planned to work on DSE after GVN (eliminating redundant loads).
It seems these two fixes may be independent, but you can check my GVN approach in the diff: https://reviews.llvm.org/D93529?id=313082.
My test case for DSE fix is

define void @foo(float* %arg, i32 %i) {
bb:
  %i7 = add nuw nsw i32 %i, 1
  %i8 = zext i32 %i7 to i64
  %i9 = getelementptr inbounds float, float* %arg, i64 %i8
  store float undef, float* %i9, align 4

Rebased, addressed comments.

@asbirlea, @nikic, would you please take a look at the updated patch?

Ping...

It seems to me this threshold bump partially compensated by cbr cost increase in all cases of unroll loops with ifs, where it is multiplicated by trip count.
This threshold bumped because of test/CodeGen/AMDGPU/unroll.ll, where started to fail

Ping...

Gentle ping...

In D93529#2517400, @asbirlea wrote:

... Could you (re-)upload a concrete use case as dependent patch?

I've just created D95543 with the test for upcoming changes in GVN. It is actually the same code as one used in unit test in this patch.

Added memory sizes to map key, BatchAA can be created without caching.

Comments addressed.

The main reason was reducing include dependencies and so source files re-builds. At the moment both GCN and R600 targets rebuild if one just changes one option in GCN related code. Actually this is the first part of split, I was thinking about splitting AMDGPUSubtarget.cpp then, and following AMDGPUMCTargetDesc.h (it currently contains both GCN+R600 tablegen output).

AA part of the splitted patch.

In D93529#2504303, @fhahn wrote:

IIUC this is similar to the handling of partial overwrites in DSE (https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp#L548)?

If so, could we just generalize the helper there, rather than threading this through the different caches from BasicAA to GVN? Yes, we have to do some extra work in GVN, but there would be no need to maintain a cached value not used by other clients, it will work with any AA implementations returning partial alias & there will be no extra work when moving away from MemDepAnalysis.

Ping. I'm going to submit the change on Friday if no objections are observed.

Ping...

Fixed include in wrong line, rebased.

Ping...

It seems patch should include update for alloca-crspill.ll test (failed Unit Tests precheckin).

Rebased, change reworked as requested.

Ping...

Splitting change as requested.

Addressed comment, fixed polly build, fixed clang-tidy comments.