Page MenuHomePhabricator

[LoopVectorizer] Fix invalid scenario that is allowed to interleave
Needs RevisionPublic

Authored by dancgr on Dec 17 2020, 11:32 AM.

Details

Summary

Tentative change to Loop Vectorizer to avoid a case of interleaving that creates incorrect code.

This problem is described on bug https://bugs.llvm.org/show_bug.cgi?id=48546.

In short, there is a case of compiling the following code with -O2 which triggers the loop vectorizer bug with clang -O2 test.c

int printf(const char *, ...);
int a, b, e;
char c, d;
int main() {
  d = 19;
  printf("%d\n", c);
  for (; d < 161; d++) {
    char *f = &c;
    *f &= 1;
    e = b== 0 ?:a%b;
    (*f)--;
  }
  printf("%d\n", c);
}

The output compilation produces incorrect code. The value printed is 254 instead of 0.

This is because we determine that we can interleave this loop, when instead it should not be allowed.

There are cases without interleaving that would make it legal to vectorize this code, so the condition is placed only if we can interleave.

Diff Detail

Event Timeline

dancgr created this revision.Dec 17 2020, 11:32 AM
dancgr requested review of this revision.Dec 17 2020, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 11:32 AM

Are there any opinions on this bugfix?

fhahn added a comment.Jan 20 2021, 6:58 AM

can you add a test case?

fhahn added inline comments.Jan 20 2021, 7:04 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
1268

Could you elaborate why the type width would matter here? Is it possible that this only exposed a problem somewhere else, e.g. lowering operations on vectors of i1?

(I tried to reproduce the problem with current trunk on macOS, but failed)

Hello

I could not replicate the bug in https://bugs.llvm.org/show_bug.cgi?id=48546 with the latest clang, as I mentioned in the ticket. Can you provide a more specific reproducer, and use it as a test case for what is being fixed?

dancgr edited the summary of this revision. (Show Details)Jan 26 2021, 12:45 PM
dancgr edited the summary of this revision. (Show Details)

@dmgreen @fhahn Sorry about that, the test case I posted was incorrect. I updated the test.c file, and in the process of finding the correct test case for you to be able to reproduce I found out the bug is worse than I expected.

With the new code I edited in the description you should be able to reproduce the bug. Unfortunately this patch doesn't cover the new test case, I am working in a new patch.

fhahn requested changes to this revision.Wed, Feb 17, 12:22 PM

Marking as changes requested, as there is an update pending IIUC

This revision now requires changes to proceed.Wed, Feb 17, 12:22 PM