Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCEV backedge-taken count assumed to be signed #17561

Closed
llvmbot opened this issue Sep 10, 2013 · 4 comments
Closed

SCEV backedge-taken count assumed to be signed #17561

llvmbot opened this issue Sep 10, 2013 · 4 comments
Labels
bugzilla Issues migrated from bugzilla polly

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2013

Bugzilla Link 17187
Resolution FIXED
Resolved on Jan 18, 2016 17:02
Version unspecified
OS Linux
Attachments Sample backedge-taken count with signed integer wrap
Reporter LLVM Bugzilla Contributor
CC @jdoerfert

Extended Description

In case of signed integer wrap of the backedge-taken count from the SCEV, Polly generate empty domains.

See attached test case:
$ opt -load LLVMPolly.so -scalar-evolution -polly-scops -analyze test.ll
Printing analysis 'Scalar Evolution Analysis' for function 'test':
Classifying expressions for: @​test
%i = phi i7 [ 0, %preheader ], [ %i.1, %body.incr ]
--> {0,+,1}<%header> Exits: -1
%tmp = zext i7 %i to i64
--> {0,+,1}<%header> Exits: 127
%A.addr = getelementptr [128 x i32]* @​A, i64 0, i64 %tmp
--> {@A,+,4}<%header> Exits: (508 + @​A)
%A.load = load i32* %A.addr, align 4
--> %A.load Exits: <>
%A.inc = zext i7 %i to i32
--> {0,+,1}<%header> Exits: 127
%A.val = add nsw i32 %A.load, %A.inc
--> ({0,+,1}<%header> + %A.load) Exits: <>
%i.1 = add i7 %i, 1
--> {1,+,1}<%header> Exits: 0
Determining loop execution counts for: @​test
Loop %header: backedge-taken count is -1
Loop %header: max backedge-taken count is -1
Printing analysis 'Polly - Create polyhedral description of Scops' for region: 'header => exit' in function 'test':
Context:
{ : }
Statements {
Stmt_body_load
Domain :=
{ Stmt_body_load[i0] : 1 = 0 };
Scattering :=
{ Stmt_body_load[i0] -> scattering[0, i0, 0] };
ReadAccess :=
{ Stmt_body_load[i0] -> MemRef_A[i0] };
MustWriteAccess :=
{ Stmt_body_load[i0] -> MemRef_A_load_s2a[0] };
Stmt_body_store
Domain :=
{ Stmt_body_store[i0] : 1 = 0 };
Scattering :=
{ Stmt_body_store[i0] -> scattering[0, i0, 1] };
ReadAccess :=
{ Stmt_body_store[i0] -> MemRef_A_load_s2a[0] };
MustWriteAccess :=
{ Stmt_body_store[i0] -> MemRef_A[i0] };
}

Reason:
SCEV of backedge-taken count is parsed wrongly assuming signed value. (See SCEVAffinator::visitConstant)
C/C++ forbid signed wrap but LLVM IR does not.

Good fix:
Better parsing of SCEV taking sign into account.

Poor fix:
Forbid backedge-taken count to have sign-wrap, at ScopDetection. (Reject if count is negative)

@tobiasgrosser
Copy link
Contributor

Patch
Merci Alexandre, this is a really nice bug report with a nice test case and a great investigation of the problem. I attached a patch that fixes your very test case, but unfortunately I don't think we can commit it as is. Specifically, it makes the domain expressions very complex even for the cases where we know there will be no integer wrapping. A way to remove this complexity for cases where the nsw flag is present is to walk the LLVM-IR and to analyze each instruction that has an nsw flag. For such instructions we derive the condition under which the nsw flags hold and add this information to the scop's context. If we now simplify the iteration domains under this context, the previously introduced modulo expressions should disappear in the common case.

I am currently too busy to look into this immediately. Though if you would like to do so, I am very happy to give feedback.

@jdoerfert
Copy link
Member

A new attempt in resolving the problem is given in:
http://reviews.llvm.org/D5728

It is based on the old patch "idea" but takes into account:

  1. That we do not want to promote everything as wrap arounds occures only in corner cases.
  2. That we cannot handle values bigger than i64 in the code generation.

@jdoerfert
Copy link
Member

We do not use ScalarEvolution backedge taken count any more to build the domain, thus this problem is "fixed".

@tlattner
Copy link
Contributor

Move to Polly Product.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 9, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla polly
Projects
None yet
Development

No branches or pull requests

4 participants