Paths

Table of Contentst

[AlignmentFromAssumptions] avoid crash on alignment constant expression
Needs RevisionPublic
Actions

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
View All

	Time	Test
	2,840 ms	x64 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 ms	x64 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 ms	x64 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 ms	x64 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 ms	x64 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

Herald added subscribers: javed.absar, hiraditya, mcrosier. · View Herald TranscriptJul 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

jdoerfert requested changes to this revision.Jul 20 2021, 8:39 AM

jdoerfert mentioned this in D94433: Do not emit non-power-of-2 alignment assume bundles (PR48713)..

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

Harbormaster completed remote builds in B115111: Diff 360142.Jul 20 2021, 9:13 AM

In D106368#2890603, @jdoerfert wrote:

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.

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

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.

In D106368#2890810, @lebedev.ri wrote:

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

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?

In D106368#2922582, @rmansfield wrote:

In D106368#2890810, @lebedev.ri wrote:

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

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:

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

Revision Contents
Changeset List

Path

Size

llvm/

lib/

Transforms/

Scalar/

AlignmentFromAssumptions.cpp

3 lines

test/

Transforms/

AlignmentFromAssumptions/

simple.ll

15 lines

Diff 360142

View Options

llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp