Page MenuHomePhabricator

[LAA] Add remarks for unbounded array access
ClosedPublic

Authored by malharJ on Dec 16 2021, 5:15 AM.

Diff Detail

Unit TestsFailed

TimeTest
60,030 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir ; /usr/bin/cmake --build . --target check-standalone | tee /var/lib/buildkite-agent/builds/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.toy.tmp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/Examples/standalone/test.toy
60,060 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,140 msx64 debian > libFuzzer.libFuzzer::large.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LargeTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/large.test.tmp-LargeTest
60,040 msx64 debian > libFuzzer.libFuzzer::out-of-process-fuzz.test
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp
60,070 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest
View Full Test Results (7 Failed)

Event Timeline

malharJ created this revision.Dec 16 2021, 5:15 AM
malharJ requested review of this revision.Dec 16 2021, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2021, 5:15 AM
malharJ updated this revision to Diff 396196.Dec 25 2021, 8:17 AM
  • Updated unit test
  • Updated minor clang-formatting errors.
malharJ updated this revision to Diff 396198.Dec 25 2021, 9:48 AM
  • Updated unit test again (had missed something earlier)
malharJ added inline comments.Dec 25 2021, 9:52 AM
llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
200

This change is to address the comment: https://reviews.llvm.org/D108371#inline-1093152

I've updated the debug info in the test to point the loop to
at least the correct source line now.
( I assume this is some error in the initial test or it has been
written by hand )

malharJ updated this revision to Diff 396855.Jan 1 2022, 5:38 AM
  • Rebased patch to acquire changes from D108371
  • minor updates in the lit test
malharJ updated this revision to Diff 397553.EditedJan 5 2022, 6:21 AM
  • Added YAML to LIT test

(This is in response to https://reviews.llvm.org/D108371#inline-1112148)

sdesmalen added inline comments.Jan 24 2022, 6:32 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
563

Instead of passing in the pointer with a default value of nullptr, can you make it a reference, so that it always gets set, e.g.

Value *&UncomputablePtr

That also removes the need for the conditional write.

llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
200

Is this an unrelated change?

malharJ updated this revision to Diff 403810.Jan 27 2022, 3:01 PM
malharJ marked an inline comment as done.
  • Rebased patch onto parent.
malharJ updated this revision to Diff 403811.Jan 27 2022, 3:06 PM
  • Minor update to unit test.
malharJ added inline comments.Jan 28 2022, 3:17 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
563

Can we please leave it as initialised as nullptr ?

The conditional write ensures that the first unsafe dependence gets reported by the remark.

if (UncomputablePtr)
    *UncomputablePtr = Access.getPointer();
llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
200

Ok. I've submitted as a separate commit to avoid any confusion.
https://github.com/llvm/llvm-project/commit/b75bdff4a0e91af1237ba77adce2f9fc7198ec26

malharJ updated this revision to Diff 403981.Jan 28 2022, 5:32 AM
  • Changed UncomputablePtr parameter to be a reference to a pointer.
malharJ added inline comments.Jan 28 2022, 6:23 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
563

Ok please ignore the above. I've made the change as suggested,

But I didn't remove that bit in the code.
The conditional write ensures that the first unsafe dependence gets reported by the remark.

if (!UncomputablePtr)
    UncomputablePtr = Access.getPointer();
malharJ updated this revision to Diff 404441.Jan 31 2022, 1:40 AM
  • trivial (clang) formatting updates
sdesmalen added inline comments.Feb 1 2022, 6:13 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
563

But I didn't remove that bit in the code.
The conditional write ensures that the first unsafe dependence gets reported by the remark.

That should be the choice of the caller, not the callee. If the caller doesn't want the value to be overwritten, it should pass a dummy variable

823

I believe this check should be removed (see my other comment as well)

2108–2110

This variable aliases with UncomputablePtr defined on line 2075. Either pass in the same variable, or give this variable a different name.

malharJ updated this revision to Diff 405368.Feb 2 2022, 11:06 AM
malharJ marked an inline comment as done.
  • removed conditional write of UncomputablePtr
malharJ marked 2 inline comments as done.Feb 2 2022, 11:07 AM
sdesmalen added inline comments.Feb 3 2022, 12:42 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2078

Before this change, recordAnalysis was called unconditionally, but now it is only called if UncomputablePtr is set. I don't think that's intentional, so you can instead just do this:

auto *I = dyn_cast_or_null<Instruction>(UncomputablePtr);
recordAnalysis("CantIdentifyArrayBounds", I) << "cannot identify array bounds";
2108

nit: s/UncomputablePtr2/UncomputablePtrUnused/

malharJ updated this revision to Diff 405557.Feb 3 2022, 2:41 AM
malharJ marked 2 inline comments as done.
  • call recordAnalysis unconditionally.
malharJ updated this revision to Diff 409190.Feb 16 2022, 3:10 AM
  • updated to pass uncomputableptr to recordAnalysis()
sdesmalen accepted this revision.Feb 23 2022, 5:50 AM
sdesmalen added inline comments.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2108–2110

nit: I'm not really a fan of the name, so personally I would prefer to reuse the old variable, e.g. reset it first:

UncomputablePtr = nullptr;

and then pass that into canCheckPtrAtRT.

This revision is now accepted and ready to land.Feb 23 2022, 5:50 AM
This revision was landed with ongoing or failed builds.Feb 23 2022, 7:58 AM
This revision was automatically updated to reflect the committed changes.

Thanks a lot for the review !

malharJ marked an inline comment as done.Feb 23 2022, 8:07 AM