llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.36k stars 11.71k forks source link

valgrind error in llvm::hasVectorizeTransformation #95157

Open dcb314 opened 3 months ago

dcb314 commented 3 months ago

From the clang testsuite at clang/test, file ./CodeGenCXX/pragma-loop-pr27643.cpp does this with recent clang++:

$ valgrind --trace-children=yes -q ~/llvm/results/bin/clang++ -c -w -O1 ./CodeGenCXX/pragma-loop-pr27643.cpp ==52393== Conditional jump or move depends on uninitialised value(s) ==52393== at 0x25A19AB: operator==<int, int> (optional:1362) ==52393== by 0x25A19AB: llvm::hasVectorizeTransformation(llvm::Loop const) (LoopUtils.cpp:406) ==52393== by 0x5E1227D: warnAboutLeftoverTransformations(llvm::Loop, llvm::OptimizationRemarkEmitter*) (WarnMissedTransforms.cpp:48) ==52393== by 0x5E12DAF: warnAboutLeftoverTransformations (WarnMissedTransforms.cpp:88)

dcb314 commented 3 months ago

This bug has existed since sometime before git hash b050048d35f6580fb427e6de9063444aa85625c6, dated 20240519.

nikic commented 3 months ago

Valgrind uninitialized value warnings have false positives (e.g. when loop unswitching is involved). Do you have any reason to believe this is not one?

As we have buildbots running with memory sanitizer, I'll generally assume that valgrind uninitialized value warnings are false positives unless there is evidence to the contrary.

dcb314 commented 3 months ago

Valgrind uninitialized value warnings have false positives

I have been using valgrind since shortly after it was invented in year 2000 and I've never seen this.

I suggest sending a bug report to the valgrind developers in this case. They will want to hear about this.

As we have buildbots running with memory sanitizer,

Memory sanitiser checks for different errors than valgrind. This is well documented in the literature.

I strongly recommend looking into the problem and confirming it as false positive, rather than just dismissing it without looking.

dcb314 commented 3 months ago

The line that valgrind complains about is:

if (Enable == true && VectorizeWidth && VectorizeWidth->isScalar() && InterleaveCount == 1)

Local variable "Enable" is unlikely to be the culprit, as it is tested against false earlier in the code.

That leaves the rest of the line as a possible candidate.

paulfloyd commented 3 months ago

Valgrind uninitialized value warnings have false positives (e.g. when loop unswitching is involved). Do you have any reason to believe this is not one?

As we have buildbots running with memory sanitizer, I'll generally assume that valgrind uninitialized value warnings are false positives unless there is evidence to the contrary.

Pmji, Valgrind developer here.

I'm not aware of any "loop unswitching" false positives. Google also hasn't heard of "valgrind loop unswitching".

It makes me sad to read comments like this. IMO you are getting things the wrong way round. If memcheck reports an error then the code is guilty until proven innocent.

paulfloyd commented 3 months ago

The line that valgrind complains about is:

if (Enable == true && VectorizeWidth && VectorizeWidth->isScalar() &&

  InterleaveCount == 1)

Local variable "Enable" is unlikely to be the culprit, as it is tested

against false earlier in the code.

That leaves the rest of the line as a possible candidate.

In order to find which variable is at fault, try the following.

Open two terminals. In the first run "valgrind --vgdb-error=0 your_exe". In the second run "gdb your_exe" and copy and paste the command that Valgrind printed in the first terminal. Then c[ontinue] until gdb breaks on the error.

gdb will probably print a message about setting the safe load path for the python helpers. I recommend making those changes then going back and repeating the previous two steps.

Now you can issue gdb commands like

mc xb &InterleaveCount sizeof(InterleaveCount)

That will print information on the initializedness of InterleaveCount. There will be two lines of values. The top line is the initializedless, 0 for initialized 1 for uninitialized. The second line is the contents of the examined memory. See https://valgrind.org/docs/manual/mc-manual.html#mc-manual.monitor-commands for more details. If you aren't using the python helper, use "mo[nitor]" as in the link to the manual rather than "mc".

dcb314 commented 3 months ago

In order to find which variable is at fault, try the following.

Thanks for your advice, but the problem seems to have gone away in recent versions of clang:

./CodeGenCXX/pragma-loop-pr27643.cpp ./CodeGenCXX/pragma-loop-pr27643.cpp:8:3: warning: loop not interleaved: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering [-Wpass-failed=transform-warning] 8 | for (int i = 0; i < Length; i++) | ^ 1 warning generated. ./CodeGenCXX/pragma-loop-predicate.cpp

To look deeper at the problem, I would have put in a lot of debugging statements and done a rebuild and re-test.

Your way looks more efficient.

paulfloyd commented 3 months ago

In order to find which variable is at fault, try the following.

Thanks for your advice, but the problem seems to have gone away in recent versions of clang:

Good. I suppose that also means it wasn't a false positive.