[CodeGenPrepare] Fix signed overflow

[mikael-nilsson-arm]

The signed addition could overflow which is undefined behavior, now the code checks for it.

[mshockwave]

is it possible to add a test?

[mikael-nilsson-arm]

I have a small test locally that triggers the issue with sanitizers enabled (the sanitizer will complain about the addition). But without it enabled I'm not sure how I could test for it.

[mshockwave]

I have a small test locally that triggers the issue with sanitizers enabled (the sanitizer will complain about the addition). But without it enabled I'm not sure how I could test for it.

Does the overflow affect codegen? Like, when it overflows TLI.isLegalAddressingMode becomes true (or the other way around). If that's the case you can write a codegen test to demonstrate it.

Also, please refrain from force-push unless it's necessary.

[mikael-nilsson-arm]

Ok, then I will not force-push.

Let's say I manage to cause a difference with and without the patch using a test. Then it's not certain the same difference (or any difference at all) can be reproduced on another compiler due to the undefined behavior. Does it really make sense to add such a test?

[mshockwave]

Ok, then I will not force-push.

Let's say I manage to cause a difference with and without the patch using a test. Then it's not certain the same difference (or any difference at all) can be reproduced on another compiler due to the undefined behavior.

True, on some platforms or compilers the codegen result might be the same regardless of your fix. But I still think it's a good idea to check in a test showing what we expect for future references.

[mikael-nilsson-arm]

I could add an LLVM_DEBUG print that conditionally print something in the case of overflow and then check for that in the lit test. Would such a a test be fine?

[mikael-nilsson-arm]

Thinking about this more I don't like the idea of adding a test that tests the debug print for something like this either. The only thing we gain is the coverage and nothing else. I still consider adding a test not worth it.

[mshockwave]

LGTM

Thinking about this more I don't like the idea of adding a test that tests the debug print for something like this either. The only thing we gain is the coverage and nothing else. I still consider adding a test not worth it.

After revisiting this, I agree that we can probably leave it without a test for this particular case.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder openmp-offload-amdgpu-runtime-2 running on rocm-worker-hw-02 while building llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/6554

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/./bin/clang -fopenmp   -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test -L /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test/ompt /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/./bin/clang -fopenmp -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test -L /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -I /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test/ompt /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# note: command had no output on stdout or stderr
# executed command: /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************