[LAA] Add remarks for unbounded array access

[malharJ]

Summary

Depends on D108371

[malharJ]

• Updated unit test
• Updated minor clang-formatting errors.

[malharJ]

• Updated unit test again (had missed something earlier)

[malharJ]

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]

• Rebased patch to acquire changes from D108371
• minor updates in the lit test

[malharJ]

• Added YAML to LIT test

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

[sdesmalen]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

564

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]

• Rebased patch onto parent.

[malharJ]

• Minor update to unit test.

[malharJ]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

564

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]

• Changed UncomputablePtr parameter to be a reference to a pointer.

[malharJ]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

564

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]

• trivial (clang) formatting updates

[sdesmalen]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

564

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

802

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

2113

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

[malharJ]

• removed conditional write of UncomputablePtr

[sdesmalen]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

2082–2085

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";

2113

nit: s/UncomputablePtr2/UncomputablePtrUnused/

[malharJ]

• call recordAnalysis unconditionally.

[malharJ]

• updated to pass uncomputableptr to recordAnalysis()

[malharJ]

ping.

[sdesmalen]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

2113

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.

[malharJ]

Thanks a lot for the review !