In D88217#2293418, @aqjune wrote:

About pointer equality propagation - @fhahn added canReplacePointersIfEqual at https://reviews.llvm.org/D85524. Would it be good if I update GVN to use this function?

Another rebase just before committing.

In D87691#2290606, @kparzysz wrote:

Fixed in https://reviews.llvm.org/rGe976fb1e54f3.

In D84679#2290130, @dmgreen wrote:

Hmm. OK. I do strong believe that we should be moving towards def-use chains, so in general I'm glad to see that happening.

I'm less convinced about the multiple inheritance. That seems like an antipattern. We can likely make it work either way, but can you try and explain why you think that way is better? As opposed to making all VPRecipe's inherit from VPValue from VPRecipeBase? Perhaps I'm too stuck in the "llvm-ir" way of thinking, with VPValue being Value, VPUser being User, VPRecipeBase being Instruction and the individual recipes being the different instructions.

I think it looks like this patch is causing EarlyCSE to crash with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17662/consoleFull#-261651283a1ca8a51-895e-46c6-af87-ce24fa4cd561. This commit is the only CSE/MemorySSA related change in the build that started crashing.

In rG8fdac7cb7abbeeaed016ef9eb7a087458e41e33f#948558, @rupprecht wrote:

The issue is with the new pass manager.

Ah that was probably why I didn't manage to reproduce it. Thank you very much for the reproducer below!

Rebased, updated comment, added additional tests.

In D84679#2286345, @dmgreen wrote:

It would be great to have some concrete simple transforms using it as soon as its ready to make sure everything works well together, so if we could collaborate on that, that would be great!

Yeah, that sounds good. vmulh worked well I though, but I ended up making some fairly major modifications. And it did need to handle costing through vplan to really get anywhere. I will see if I can come up with anything for that too. I think just having a conversation about some of these design details is helpful in itself.

Address Ayal's comments, thanks!

In D87867#2285957, @thegameg wrote:

I am totally fine with keeping this in-tree, even unused, if it helps several out-of-tree backends. Thanks for speaking up.

Maybe someone using it can provide an explanation of potential uses of this node as a comment in ScheduleDAG.h?

It would be great if you could share more details about how to reproduce the failure. I tried to reproduce with a stage-2 build with -DLLVM_ENABLE_MODULES=On, but check-clang passed. Could you share the file/IR causing the crash?

It looks like it is indeed possible to generate fixup values that wrap around in valid LLVM IR and truncating the computed value to the fixup width should match the computation in LLVM IR.

In D87890#2287035, @mkazantsev wrote:

It seems that there is no way that SCEV can now both make this query and return true for it. I'm currently building a chain of patch that will fix the query responds. Hopefully with them we will be able to construct a test for it.

Or can we just merge it basing on obviousness of this logic?

In D87972#2284060, @MaskRay wrote:

I have tested this patch internally and seen gains and losses. On one document search related benchmark 3~5% improvement. One zippy (snappy) there is 3~5% regression. Perhaps we do need a conditional extra SROA run.

In D87163#2285984, @dmajor wrote:

I threw as many tests as I could find at 9d172c8e9c84, and I don't see any regressions compared to its parent revision.

In D84679#2285752, @dmgreen wrote:

With this patch, VPRecipeBase is also a VPUser and a VPValue, it just inherits from both directly, rather than indirectly through VPValue. Note that 'replacing all uses with & co' are provided by VPValue, so all instances that could previously be RAUW'd can still be RAUW'd. The only major difference should be that in the future we may need some extra code to get the uses of a VPUser.

Do you mean VPInstruction is a VPUser and a VPValue? I really meant that _all_ recipes were VPValues and VPUsers, moving it to VPRecipeBase.

Sorry, I did not mean that everything will be changed in this patch. But the linked patches gradually turn all the recipes into VPValues and update VPInterleaveRecipe to produce multiple values (and moves VPlan codegen to use that). So with the current patch alone, it is not yet possible to traverse def-use chains in recipes, but once the transition is done, it should be possible to traverse in both directions. (I think most recipes should already be ready to be turned into VPUser, let me check which ones are still missing)

In D67178#2280288, @reames wrote:

LGTM. This makes a nice starting point - as you note, we can definitely extend this.

This looks good to me, as part of the effort to support VPlan def-use modeling and traversals.
Note that top-down traversal from VPValue to its VPUsers, will now need to check if each VPUser isa single-def recipe/VPInstruction in order to continue downwards to its VPUsers, etc., facilitating multi-def recipes.

Turns out that things actually go slightly wrong earlier during LSR. LSR did not pass MSSU to SplitCriticalEdge, which caused the MemoryPhi to have slightly wrong incoming blocks. I pushed a fix for that, as it is straight-forward to fix (57ae9bb9323548b2ad4ba8274c3910bf9c764983). With that, this patch is no longer needed for the test case.

In D82895#2283534, @baziotis wrote:

@baziotis Are you going to update this diff?

Thanks, I had forgotten about this.

@fhahn OK with renaming empty() to isInnermost()?

I'd say that now we see all the places empty() was used, for most of them the name isInnermost() makes more sense.

LGTM, thanks for also improving the test case!

Surprising this causes only such a small perf regression. I guess it should be OK given that, but here probably are some pathological cases out there where this may cause some noticeable compile-time regressions.

LGTM, thanks!

Could you also add a test for BasicAA, in llvm/test/Analysis/BasicAA/libfuncs.ll?

expand comment stating why it should be safe to use GEP of null instead of inttoptr as Eli suggested, thanks!

Great. Please let us know if you need someone to commit the change on your behalf.

LGTM thanks.

This looks indeed unused, thanks for cleaning this up! There may be downstream targets that make use of EntrySU, but if that's the case it is easy to revert.

Output of failed test:

It appears that after this change, use_registers.cpp is failing on green dragon: http://green.lab.llvm.org/green/job/clang-stage1-RA/14618/consoleFull#-1417328700a1ca8a51-895e-46c6-af87-ce24fa4cd561

Looks like this still hasn't landed and probably requires at least a rebase.

(marking as changes requested to indicate that there's an outstanding comment)

(marking as changes requested to indicate that there's an outstanding comment)

(marking as changes requested to indicate that there's an outstanding comment to clear up review queue)

In D72148#2242386, @fhahn wrote:

In D72148#2242301, @ebrevnov wrote:

I won't have time to look into this in the near future unfortunately, as I am mainly focused on getting things in good shape to enable DSE & MemorySSA by default. It might be good to file a bug report so this opportunity does not drop off our radar. If anyone is interested in giving implementing it a try, even better :)

Florian, thank you for quick feedback. I prototyped phi translation approach and it does work on the posted test case. Unfortunately, it fails on a bit more complicated case which I will take a look tomorrow. If I have something working reasonably well in the end I will share it.

That's great! There are some changes to how we find MemoryDefs that may be killed by a killing def, rGe717fdb0f155 and D86487. This might actually make things a bit easier, as we now fully control the traversal of the defining accesses.

I finally had some time to address the outstanding comments. But given that there seems to be no consensus on llvm-dev that people actually want this, I won't try to push this any further for now.

Thanks for the patch! Could you also add a test case where src dominates dest?

All reported problems should be fixed now and we also made the fix for the issue @dmajor reported more conservative to catch more problematic cases. I just enabled DSE + MemorySSA again in 9d172c8e9c84. Please let me know if there are any further issues.

In D87827#2280309, @efriedma wrote:

I have a proposal in https://bugs.llvm.org/show_bug.cgi?id=46786#c20 which would solve this, among other issues. Not sure when I'll get around to actually implementing it.

Change generated code to use bitcast (GEP i8* null, %v) to Ty. Also adds an assertion that i8 has an alloc size of 1 byte.

Would it be worth adding a C++ unit test for those cases that just passes in suitable predicates + SCEV expression?

Apply suggested change, set size to unknown if result of phi translation is not invariant.

Finally had some time to get back to this one. Rebased and also added SCEV test for PR40961. I will also post a follow-up that extends this to also support assumes to catch PR47247.

rebase & resurrect.

oh, it might be good to slightly adjust the commit message to be a bit more compact and also mention what actual bug is fixed, e.g. something like [AArch64] Avoid paring loads with same result reg.

use precise size for translated addr.

LGTM, thanks!

Add accidentially dropped include of User.h

In D87163#2276899, @dmajor wrote:

In D87163#2275896, @asbirlea wrote:

I checked in a fix in https://reviews.llvm.org/rGfc8200633122, but I haven't yet verified it addresses all the failures reported.

Thanks, I've confirmed that this fixes our tests in their original form (not reduced).

Our day-to-day testing of LLVM trunk is limited though, maybe one-tenth of our full suite. Since this code has some risks, I could start a full run to throw more testing at it. But that takes more machine capacity and human effort to remove flaky failures, so I'd prefer to wait until the odds are good that there won't be further changes. Let me know when you think it's ready.

In D86879#2274099, @jasonliu wrote:

In D86879#2274076, @nemanjai wrote:

In D86879#2274025, @hubert.reinterpretcast wrote:

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

Perhaps the test case should be restricted to some key platforms where cross-compilation to AIX is relevant? Perhaps Linux (PPC is already required).

I could add
# REQUIRES: aix || linux
to the test if that helps.

rebase & ping

rebase & ping

Rebase & ping

In D87132#2275913, @tdangeti wrote:

In D87132#2273405, @fhahn wrote:

LGTM, thanks for fixing this issue!

Thanks for reviewing.
I don't have commit rights. Could you please commit on my behalf?