LGTM! Thanks for the bug fixing

In D116276#3212829, @nikic wrote:

@guopeilin This doesn't really answer my question. You say that the SCEV for the wide IV at this point is {((sext i32 %arg1 to i64) + (sext i32 %arg2 to i64)),+,(sext i32 %arg2 to i64)}<nsw><%body>, which has the <nsw> flag. My baseline expectation would be that SCEVExpander will also add the nsw flag to the expanded IR (see the code around https://github.com/llvm/llvm-project/blob/015ff729cb90317e4e75cf48b1e5dd7850f0cbd0/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp#L1300-L1304). My question is why that doesn't happen because making sure SCEVExpander propagates the flag from SCEV to IR seems like the more principled way to address this problem.

In D116276#3209662, @nikic wrote:

Can you explain why SCEVExpander does not preserve the nowrap flag in this case? Assuming it is present on the wide IV, I would have expect it to also get expanded as such.

Sorry for the late reply. The SCEVExpander(Rewriter) is used to create the widen IV and its widenIVUse(). As for the IV increment, we just use the following code:

WideInc =
      cast<Instruction>(WidePhi->getIncomingValueForBlock(LatchBlock));

Of course in this way, we cannot preserve the nowrap flag cause both the WidePhi and the IncomingValueForBlock do not contain the flag.
I guess the reason why we don`t need to preserve this flag previously is that the AddRec is computed from the OrigPhi, which is like the following:

{((sext i32 %arg1 to i64) + (sext i32 %arg2 to i64)),+,(sext i32 %arg2 to i64)}<nsw><%body>

So, at this moment, it does not matter whether the increased instruction contains the flag because the SCEV is right.
However, during the optimization pipeline, we may call the SE->forgetLoop() to drop the cache value and recompute from scratch. At that moment, since we have lost the NSW flag, then the BackedgeTakenCount would be CouldNotCompute, which will prevent vectorizing.
With this patch, function s122 and function s172 in TSVC now can be vectorized. Following is the source code of s122:

real_t s122(struct args_t * func_args)
{
//    induction variable recognition
//    variable lower and upper bound, and stride
//    reverse data access and jump in data access
    struct{int a;int b;} * x = func_args->arg_info;
    int n1 = x->a;
    int n3 = x->b;
    initialise_arrays(__func__);
    int j, k;
#pragma clang loop vectorize(assume_safety)
    for (int nl = 0; nl < iterations; nl++) {
        j = 1;
        k = 0;
        for (int i = n1-1; i < LEN_1D; i += n3) {
            k += j;
            a[i] += b[LEN_1D - k];
        }
    }
}

fix the affected tests

With the help of nsw flag in keep-nsw-nuw-flag.ll, this loop which cannot be vectorized due to CouldNotCompute BackedgeTakenCount now can be vectorized.
We can use opt -indvars keep-nsw-nuw-flag.ll -S | opt -loop-vectorize -S to verify this.
Also, if we just use -indvars -loop-vectorize back to back, this loop can be vectorized without this patch. And I found out that the reason is that nsw flag still remains when we try to vectorize this loop. That is some cached value is reused during scalar-evolution analysis for loop-vectorize pass.
During the pipeline, we may call forgetLoop in many places, thus the cached value may be cleared. So I guess it is better to keep the nsw or nuw flags explicitly after widen IV.

Sorry for the absence of the description.
Actually, it is a runtime error. The simplify range check optimization will change (icmp slt x, 0) | (icmp sgt x, n) into icmp ugt x, n
Now, suppose the %0 is negative, Before optimizing, we will compare the argument with 0 to see whether the argument is less than 0, if so, we will finally return shl i32 %0, 0.
However, after optimizing, we will not have this comparison, and we will finally return shl i32 %0, %and. The problem is that %and is also driven from the argument, it can be undefined cause it is a shift operation that depends on the argument. So, after optimization, we will get an undefined result.
So, I guess that if both x and n in (icmp slt x, 0) | (icmp sgt x, n) are derived from the argument, then we should be conservative.

update test file

pinging reviewers...

In D110524#3027244, @david-arm wrote:

Hi @guopeilin, thanks for making the changes. I'm still a little concerned about the test case though as I'm not sure how reliable it will be over time, especially with the undef values present in the code. I did manage to reduce this test case to:

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mattr=+sve -aarch64-sve-vector-bits-min=256 < %s | FileCheck %s
target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64-unknown-linux-gnu"

define <4 x i32> @test_work_knt_val(<16 x i32>* %arg) {
entry:
  %0 = load <16 x i32>, <16 x i32>* %arg, align 256
  %shvec = shufflevector <16 x i32> %0, <16 x i32> undef, <4 x i32> <i32 14, i32 14, i32 14, i32 14>
  %1 = add <16 x i32> %0, %0
  store <16 x i32> %1, <16 x i32>* %arg, align 256
  ret <4 x i32> %shvec
}

I think the problem is that there has to be multiple uses of the loaded value (%0) in order for the DAG combine to trigger.

Tried to simplify the test case but failed. @david-arm

Pinging reviewers ...

In D108755#2975020, @paquette wrote:

LGTM

Pinging reviewers...

Hi, @sammccall thanks for the patch.
The precommit checks suggest that some test cases failing, could you please fix them. Thanks

Hi all!
Updata a new test case.
Function test2 will fail due to assertion of invalid offset of EXTRACT_SUBVECTOR.
Also modify the AMD test case

Pinging reviewers....

In D82657#2956851, @sammccall wrote:

In D82657#2956751, @guopeilin wrote:

Hi @hokein , I encounter a bug when clang parses enum and I have been recorded in https://bugs.llvm.org/show_bug.cgi?id=51554.

I added a comment on the bug and sent https://reviews.llvm.org/D108451

This is a "rough edge" between erroneous and well-formed code and I'm not totally sure it's the perfect fix, but it does seem to be robust against crashes which is definitely he biggest thing.

Hi @hokein , I encounter a bug when clang parses enum and I have been recorded in https://bugs.llvm.org/show_bug.cgi?id=51554.
The source code like the following:

enum E { e = E() };
int main() {
  return 0;
}

Some error message are expected like the following:

test.cpp:1:14: error: invalid use of incomplete type 'E'
enum E { e = E() };
             ^~~
test.cpp:1:6: note: definition of 'E' is not complete until the closing '}'
enum E { e = E() };

Also, I have made some analyses like the following:

In ParseDecl.cpp:
1. llvm-10: 
   AssignedVal.get(): NULL

In D108123#2951595, @lebedev.ri wrote:

In D108123#2946803, @lebedev.ri wrote:

In D108123#2946790, @guopeilin wrote:

In D108123#2946738, @lebedev.ri wrote:

In D108123#2946733, @guopeilin wrote:

In D108123#2946665, @lebedev.ri wrote:

This test already passes without the code change: https://godbolt.org/z/48TnbeKj1

The store is not expected, which will become unreachable after SimplifyCFG.

Right, but that is the correct outcome.
Why is it not UB to free an undef (a.k.a. any random pointer)?
It is as per the current langref: https://alive2.llvm.org/ce/z/jBqe-E

Also, i would like to note that the patch's description doesn't mention that this wants to make free(undef) non-UB.

The key point is that function free can be overload, take this test case as an example, we can overload function _ZdlPv with an empty body. So the result becomes that we call a free function which actually does not free anything. Absolutely this is not a way that free an undef.
So it is better to delete an instruction that calling an empty function with an undefined argument.
I will update this test case later, also the description.

Thanks, but this still does not sound good to me.
If you want to change semantics of free()/_ZdlPv() in such a way,
what you need to do is to mark it as nobuiltin: https://godbolt.org/z/a5P3TzG7T

Hi all, I update a new patch with a much simple test case. Please review, Thanks a lot!

In D108123#2946738, @lebedev.ri wrote:

In D108123#2946733, @guopeilin wrote:

In D108123#2946665, @lebedev.ri wrote:

This test already passes without the code change: https://godbolt.org/z/48TnbeKj1

The store is not expected, which will become unreachable after SimplifyCFG.

Right, but that is the correct outcome.
Why is it not UB to free an undef (a.k.a. any random pointer)?
It is as per the current langref: https://alive2.llvm.org/ce/z/jBqe-E

Also, i would like to note that the patch's description doesn't mention that this wants to make free(undef) non-UB.

In D108123#2946665, @lebedev.ri wrote:

This test already passes without the code change: https://godbolt.org/z/48TnbeKj1

Within the CodegenPrepare::tryToSinkFreeOperands, those Ops that use in the same BB as TargetBB will be skipped.

for (Use *U : reverse(OpsToSink)) {
    auto *UI = cast<Instruction>(U->get());
    if (UI->getParent() == TargetBB || isa<PHINode>(UI))
      continue;
    ToReplace.push_back(U);
  }

Thus for the Ops shuffle and insertelement of Mul generated by shouldSinkOperands, if the shuffle is already in the same BB of Mul, we will not sink the shuffle. However, the insertelement instruction will be sink right above the Mul instruction while behind the shuffle instruction. That is illegal cause def not dominate the use.

Sometimes the CurrList may have two identical instructions that will be coalesced later. Once the first one coalesced, it will be removed from its parent, this will make the second identical instruction become illegal at the same time which getParent will be a nullptr. In this case, we should avoid coalescing erased instruction.<br>
The test case will trigger an Assertion like the following:

llvm-project/llvm/include/llvm/CodeGen/MachineOperand.h:359: llvm::Register llvm::MachineOperand::getReg() const: Assertion `isReg() && "This is not a register operand!"' failed.

Also recorded in Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=50919

Actually, this transformation doesn't take too much time. However the IR it generated is very long. And this will have a great effect on the further pass. More specifically, the expanding SCEV which replaces the variable %add.i.2.i contains thousands of operands.
So if you use opt -indvars -S, you can see the program keeps printing the IR all the time.
Besides, I use clang -O3 test.cpp and find that the program gets a segment fault with a stack dump like the followings:

In D103007#2776598, @lebedev.ri wrote:

I'm not a fan of this direction.
Which stack overflow are you seeing?

In D103007#2776571, @nikic wrote:

Why is this not already covered by the isHighCostExpansion() check?

Details are recorded in https://bugs.llvm.org/show_bug.cgi?id=50442.