In D94576#2495788, @lebedev.ri wrote:

(cc @spatel, re rG0aa75fb12faa04e07ba1a6e334605357b6a159c9)

Please upload patch with context:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Adding more potential reviewers based on D88669 (where this code was added).

In D94014#2490458, @nlopes wrote:

Another way is to use assume to encode the precondition if you prefer: https://alive2.llvm.org/ce/z/__szVL

Anyone see problems with this Alive2 implementation using count-leading-*?
https://alive2.llvm.org/ce/z/SWxadd

In D94014#2490113, @lebedev.ri wrote:

alive1 does not actually have a countLeadingOnes() precondition

Ping.
Note: I added a bitwidth > 2 constraint to be safe and avoid degenerate narrow bit patterns, but I'm not sure how to write negative tests for that would actually survive to this point in instcombine (instsimplify appears to reduce those first).

In D89697#2483676, @yubing wrote:

No regression appeared in our internal testcases.
It seems the transform is correct, have you verified it with alive-tv?

LGTM - see inline for possible missed comment.

In D93891#2481354, @steven.zhang wrote:

Hmm, so we can try to remove those parts that not relative with the parameter arsenm mentioned. I guess most of the check in the DAGCombiner could be removed if I understand correctly. To remove it completely, some work is needed for parameters or others that current IR cannot represent the semantics of the global setting. Is it right ?

Maybe, we can start it with this simple patch first ?

I haven't followed the details enough to comment on the changes directly, but thanks for the cleanup! The mismatched signed/unsigned cost model APIs are/were a mess.

LGTM - other reviewers are probably more familiar with InterleavedAccess than me, so might want to wait for a 2nd opinion.
I made another assert suggestion for that code. Similar to the assert that you have proposed in this patch, we could add the asserts before this patch just to make the shuffle assumptions clearer (assuming those are correct assertions).

Does this work with scalars too? Let me know if I missed the test(s).

Patch updated:
Fixed semi-colon typos in test comments.

In D93860#2475454, @pengfei wrote:

Yep, that's definitely wrong. But I think FMF propagation is not an easy thing. We cannot always copy the original FMF, right? E.g. reassoc reduction may result in inf which "sequential" doesn't. Should ninf be propagated or not after reduction?

LGTM - please pre-commit the test with the current (trunk) codegen, so we see the diff (and don't lose the test in case we have to revert).

In D93860#2475145, @spatel wrote:

SLP requires fast as a pre-condition before matching a reduction (although I'm not sure if that implementation is completely sound), so the hard-coded logic:

Unsafe.setFast();
Builder.setFastMathFlags(Unsafe);

...is theoretically safe here. But yes, that is one bug I am hoping to fix. LoopVectorizer appears to have different bug(s).

In D93860#2475122, @pengfei wrote:

In D93860#2475028, @spatel wrote:

In D93860#2473348, @pengfei wrote:

Hi Sanjay, pairwise reduction seems not adopted by any targets. But do we need to consider the FMF here? I found LangRef says the intrinsic is sequential for fadd and fmul if reassoc flag is not set.

Hi @pengfei -
Sorry for the delayed reply - I did not get an email notification for your comment here in Phab.

We do not need to care about FMF directly here because neither of these shuffle variations corresponds to "sequential" reduction of the elements. In other words, we should require reassoc (at the least) to form an fadd or fmul reduction that is expected to need any shuffles. Let me know if you see any holes in that theory.

One motivation for this cleanup is to correct bugs with using/propagating FMF (that was also true for D87416). For example - https://llvm.org/PR35538

Thanks for explanation. I was confused it with reduction intrinsics. I noticed tryToReduce sets fast flag instead of requires. Is that the problem you mentioned? By the way, can we build reduction intrinsics directly here instead of doing shuffle?

In D93891#2474602, @steven.zhang wrote:

In D93891#2473825, @spatel wrote:

We're trying to (very slowly...) move away from needing the global/target options. Also, I'm not sure how those translate with GlobalISel.
If there's a real-world case where the target options do not match the fast-math-flags on the IR, we should investigate if that was unexpected. Otherwise, I would prefer that we not add to the uses of target options.

Hmm, do you know the specific reason that it is removed so slowly ... (What I imagine is just removing it and update some tests. Some targets still want it ?) It is not right to me that it is handled inconsistent. i.e.
FPOW didn't check the unsafe-fp-math while SQRT check it.

In D93860#2473348, @pengfei wrote:

Hi Sanjay, pairwise reduction seems not adopted by any targets. But do we need to consider the FMF here? I found LangRef says the intrinsic is sequential for fadd and fmul if reassoc flag is not set.

See inline comments to avoid bot failures - otherwise, LGTM. Thanks for the cleanup!

We're trying to (very slowly...) move away from needing the global/target options. Also, I'm not sure how those translate with GlobalISel.
If there's a real-world case where the target options do not match the fast-math-flags on the IR, we should investigate if that was unexpected. Otherwise, I would prefer that we not add to the uses of target options.

In D93586#2467248, @aqjune wrote:

There are 792 files in llvm/test, most of which are in test/Transform (119) and test/CodeGen(655).
The transition will be swiftly done (if there's no other issue hopefully) by the next weekend.

Thinking about these again, do we need to make a poison copy for test/CodeGen? I don't think so, since the backend won't be changed anyway.

LGTM - there's also a code pattern like this in DAGCombiner::rebuildSetCC(), so it's hopefully safe. But as noted, we can probably do better by changing the structure a bit.

LGTM

Please review the comments in D16337 - especially:
https://reviews.llvm.org/D16337#331117

LGTM - the only diff from the earlier rev is that we now check the dominator tree to confirm that the shift ops are safe to hoist.

Thank you for working on this! Looking back at the bug comments (and adding reviewers based on those comments), this is a step towards killing undef that has been discussed for a long time now. :)

Patch updated:
Adjusted order/spelling of offset checks as suggested. Also added a test (gep012) just below the offset cut-off, so we have positive and negative tests at the boundary condition.

In D93229#2459265, @aqjune wrote:

In D93229#2457696, @spatel wrote:

We can use an alternate plan to make incremental progress, but it will cause redundancy while we transition:

1. Replicate all of the regression tests with the poison constant instead of undef. If we add some unique TODO text marker on all of those new tests, we can then grep to make sure everything that we expect to get updated is actually updated in later steps.
2. Update instcombine/simplify/vectorizer folds to match poison patterns (create a m_UndefOrPoison() pattern matcher?)

m_Undef already matches poison because PoisonValue is a subclass of UndefValue :)

3. Run Alive2 through all of the updated regression tests to verify.
4. Update codegen to deal with poison constant?
5. Update folds from step 2 to create poison rather than undef (if that's what they were doing)?
6. Change IRBuilder or other instruction creators to create poison from the start.

I think we can split the goal and first work on replacing the placeholder value that is used by insertelement only.
In this case, actually InstSimplify/InstCombine change isn't necessary as well. It can be done only when suboptimal assembly is being generated.
What do you think?

Patch updated:

1. Added constraints on address offset with respect to vector element size.
2. Added negative tests for those.
3. Added asserts to make sure address assumptions are valid.
4. Added code comment about offset math (negation) logic.

In D93065#2460747, @congzhe wrote:

However, on internal benchmarks I see much less degradation from the freeze solution compared to this patch.

Does this sound like a clue that we should go ahead with the freeze solution? If so I'll post the freeze solution and see how we can go from there.

Code changes look fairly mechanical at this point, so if the output is as expected, LGTM

Patch updated:
D93397 / D93406 improved the basic alignment logic, so that is adjusted here to include the effect of gep offset.
In the last test diff, note that the final alignment is not the same as either of the starting alignments.