This is an archive of the discontinued LLVM Phabricator instance.

Filter UnDef size values in Scop Detection
ClosedPublic

Authored by cs14mtech11017 on Jul 22 2015, 12:59 PM.

Details

Summary

Currently, there is an assert on undef size value in polly::MemoryAccess::foldAccess().
It is better to restrict them in ScopDetection itself.

Diff Detail

Event Timeline

cs14mtech11017 retitled this revision from to Filter UnDef size values in Scop Detection.
cs14mtech11017 updated this object.
cs14mtech11017 added a reviewer: grosser.
grosser edited edge metadata.Jul 22 2015, 1:14 PM

Hi Utpal,

thanks for your work.

The patch itself looks good, but could you please include a test case in
the patch. Also, please run make polly-check-format to see if your formatting
is correct or run make polly-update-format to fix it if it did not match
the LLVM style already.

Best,
Tobias

Also, please add the next time llvm-commits and polly-dev as a subscriber.

Hi Utpal,

thanks for your work.

The patch itself looks good, but could you please include a test case in

Where can I upload the test case?

the patch. Also, please run make polly-check-format to see if your formatting
is correct or run make polly-update-format to fix it if it did not match
the LLVM style already.

Yes there was differences in formatting. I have corrected it. Thanks for letting me know.

Best,
Tobias

Hi Utpal,

thanks for your work.

The patch itself looks good, but could you please include a test case in

Where can I upload the test case?

Add it to the test/ directory in Polly and make sure it passes 'make check'.

Then commit it to your git repository and upload a new patch that contains both your code changes as well as your test case to phabricator.

Best,
Tobias

Hi Tobias,

Some test cases are failing with this patch and I am stuck trying to fix one of them. Please see my details below-

With this undef patch, 24 regression test cases are failing.

  1. Run make check-all

24 test cases are failing.

  1. One among them is Polly :: ScopInfo/multidim_srem.ll
  1. Run llvm-lit (with the patch)

llvm-lit -v tools/polly/test/ScopInfo/multidim_srem.ll

The output is as attached below -

Command Output (stderr):

/home/cs14mtech11017/llvm_latest/llvm/tools/polly/test/ScopInfo/multidim_srem.ll:11:10: error: expected string not found in input
; CHECK: [n] -> { Stmt_for_body_8[i0, i1, i2] -> MemRef_A[o0, i1, i2] : exists (e0 = floor((-i0 + o0)/2): 2e0 = -i0 + o0 and o0 >= 0 and o0 <= 1) };

^

<stdin>:26:2: note: scanning from here
[n, p_1, p_2] -> { Stmt_for_body_8[i0] -> MemRef_A[o0] : 4o0 = 4p_1 + p_2 + 4i0 };
^

  1. I compared the output of this test case with and without the patch. In both the cases, the output is exactly the same. I am confused why llvm-lit is failing with the patch. Please see the two outputs below.

polly-opt -analyze -polly-scops -S ~/llvm_latest/llvm/tools/polly/test/ScopInfo/multidim_srem.ll

a. Output with patch
Statements {

    Stmt_for_body_8
        Domain :=
            [n] -> { Stmt_for_body_8[i0, i1, i2] : i0 >= 0 and i0 <= 199 and i1 >= 0 and i1 <= -1 + n and i2 >= 0 and i2 <= -1 + n and n >= 1 };
        Schedule :=
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> [i0, i1, i2] };
        ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> MemRef_A[o0, i1, i2] : exists (e0 = floor((-i0 + o0)/2): 2e0 = -i0 + o0 and o0 >= 0 and o0 <= 1) };
        MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 0]
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> MemRef_A[o0, i1, i2] : exists (e0 = floor((-i0 + o0)/2): 2e0 = -i0 + o0 and o0 >= 0 and o0 <= 1) };
}

b. Output without patch
Statements {

    Stmt_for_body_8
        Domain :=
            [n] -> { Stmt_for_body_8[i0, i1, i2] : i0 >= 0 and i0 <= 199 and i1 >= 0 and i1 <= -1 + n and i2 >= 0 and i2 <= -1 + n and n >= 1 };
        Schedule :=
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> [i0, i1, i2] };
        ReadAccess :=       [Reduction Type: NONE] [Scalar: 0]
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> MemRef_A[o0, i1, i2] : exists (e0 = floor((-i0 + o0)/2): 2e0 = -i0 + o0 and o0 >= 0 and o0 <= 1) };
        MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 0]
            [n] -> { Stmt_for_body_8[i0, i1, i2] -> MemRef_A[o0, i1, i2] : exists (e0 = floor((-i0 + o0)/2): 2e0 = -i0 + o0 and o0 >= 0 and o0 <= 1) };
}

With your patch I get the following output:

Statements {
	Stmt_for_body_8
        Domain :=
            [n, p_1, p_2] -> { Stmt_for_body_8[i0] : i0 >= 0 and i0 <= -1 + n };
        Schedule :=
            [n, p_1, p_2] -> { Stmt_for_body_8[i0] -> [i0] };
        ReadAccess :=	[Reduction Type: NONE] [Scalar: 0]
            [n, p_1, p_2] -> { Stmt_for_body_8[i0] -> MemRef_A[o0] : 4o0 = 4p_1 + p_2 + 4i0 };
        MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 0]
            [n, p_1, p_2] -> { Stmt_for_body_8[i0] -> MemRef_A[o0] : 4o0 = 4p_1 + p_2 + 4i0 };
}

This means the delinearization fails.

I checked your code. It is wrong. The 'return false' is not part of the innermost condition, as you forgot
to add the braces around the if and the if consequently only includes the next instruction:

if (auto *Unknown = dyn_cast<SCEVUnknown>(DelinearizedSize)){
     auto *value = dyn_cast<Value>(Unknown->getValue());
    ​ if (isa<UndefValue>(value))
​         invalid<ReportDifferentArrayElementSize>(Context, /*Assert=*/true,
​                                                                   Context.Accesses[BasePointer].front().first, BaseValue);
         return false  << Not part of if
}

Yes I saw that now. Thanks for catching it so quickly. Now all the test cases are passing.

OK. Can you then please update your patch and also include a test case, such that we can commit both.

Thank you,
Tobias

cs14mtech11017 edited edge metadata.
cs14mtech11017 added subscribers: llvm-commits, pollydev.

Filter UnDef size values in Scop Detection.

grosser accepted this revision.Jul 29 2015, 2:44 AM
grosser edited edge metadata.

LGTM.

I will commit this as soon as the LLVM servers are back (they seem to be done ATM).

This revision is now accepted and ready to land.Jul 29 2015, 2:44 AM
This revision was automatically updated to reflect the committed changes.