Page MenuHomePhabricator

[AlignmentFromAssumptions] avoid crash on alignment constant expression
Needs RevisionPublic

Authored by spatel on Jul 20 2021, 8:20 AM.

Details

Reviewers
nikic
jdoerfert
Summary

I'm not familiar with this pass, but we have a crash shown in:
https://llvm.org/PR50978
...so I think we can either bail out as shown here or dyn_cast later to avoid the assert failure in getNewAlignmentDiff() that happens with the plain cast<SCEVConstant>.

Diff Detail

Unit TestsFailed

TimeTest
2,840 msx64 debian > libarcher.barrier::barrier.c
Script: -- : 'RUN: at line 14'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/barrier/barrier.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/barrier/Output/barrier.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/barrier/Output/barrier.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/barrier/Output/barrier.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/barrier/barrier.c
2,830 msx64 debian > libarcher.critical::critical.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c
3,190 msx64 debian > libarcher.parallel::parallel-simple.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp -latomic && env OMP_TOOL_VERBOSE_INIT=stderr env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp.log 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple.c --check-prefixes CHECK,TSAN_ON
2,620 msx64 debian > libarcher.parallel::parallel-simple2.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple2.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple2.c
2,630 msx64 debian > libarcher.races::critical-unrelated.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/critical-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/critical-unrelated.c
View Full Test Results (20 Failed)

Event Timeline

spatel created this revision.Jul 20 2021, 8:20 AM
spatel requested review of this revision.Jul 20 2021, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2021, 8:20 AM

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

This revision now requires changes to proceed.Jul 20 2021, 8:39 AM

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

Wouldn't that be revert of https://reviews.llvm.org/D73996 which softened it to a warning then?

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

Wouldn't that be revert of https://reviews.llvm.org/D73996 which softened it to a warning then?

Oh, i forgot that i had such a patch.
That seems to list a reason why clang does what it does,
I guess then we need two changes:

  1. don't use alignment assume bundle to store non-power-of-two alignment, because that is obviously not an alignment
  2. verifier to enforce that alignment assume bundle must contain powers of two.