just resize the cache, dont assert equality of sizes

[Benoit]

Summary

This assertion has been failing on some large files --- not trivial to extract a small testcase for, but I did log the values being compared and they were Size==64 vs Cache->size()==1. I took a look at the code but wasn't able to understand why the code thought that the condition being asserted here should necessarily be true. It feels just incidental that that condition had been true so far?

[nikic]

This looks like a consistency assertion that an <n x %ty> vector has an n-element scatter in the cache -- so it's not obvious to me that just dropping the assertion is fine, I think you might be hiding a legitimate issue here.

This assertion has been failing on some large files --- not trivial to extract a small testcase for

Extracting a test case should be easy with some combination of llvm-reduce and/or reduce_pipeline.py?

[Benoit]

In D122026#3393235, @nikic wrote:

This looks like a consistency assertion that an <n x %ty> vector has an n-element scatter in the cache -- so it's not obvious to me that just dropping the assertion is fine, I think you might be hiding a legitimate issue here.

Thanks, ok let's dig deeper! Question to start really understanding this code. This CachePtr points to a ScatterMap. That type is defined here as:

using ScatterMap = std::map<Value *, ValueVector>;

So the keys are not Value's --- they are pointers to Value's.

This seems to be assuming that any Value used as a key will outlive this cache. Otherwise, another unrelated Value might get created at the address of a defunct Value that was a key in this cache, and the cache would get confused. What is ensuring that that isn't the case?

Extracting a test case should be easy with some combination of llvm-reduce and/or reduce_pipeline.py?

It's complicated because I'm only reproducing this in a dependent project that has its own MLIR-based compiler, and that creates the scalarizerPass in C++. I dumped the IR just before running Scalarizer, put it here, but i don't know how to reproduce the issue with opt. I tried,

./bin/opt -passes='function(scalarizer)' -scalarize-load-store -S /tmp/a.ll

but scalarizer isn't doing anything on this.

[Benoit]

OK, I have dug deeper now :-)

Here is the dumped function at the point where the assert is triggering. It's still not a testcase because it references types that opt doesn't know about.

I know specifically the part that is tripping the assertion, it's:

%233 = extractelement <16 x <64 x i32>*> %bc, i64 0
%wide.vec = load <64 x i32>, <64 x i32>* %233, align 4, !dbg !20

The extractelement is handled by ScalarizerVisitor::visitExtractElementInst, which calls gather, which associates to the Value %233 a vector of length 1, explicitly here.

But then the load is handled by ScalarizerVisitor::visitLoadInst , calling scatter on its operand %233, which determines that its type being <64 x i32>, its vector size is 64, thus complaining that the cached ValueVector has size 1 (from what the above gather did), not 64.

I'm ignorant enough that it's not trivial for me to turn this into a testcase (I just dont know LLVM IR as a language at all), but I could try a bit harder... maybe you have thoughts already at this point, though.

[Benoit]

The above 2 lines really were a full testcase -- just needed to wrap that in a function!

Filed this github issue with the testcase and debugging details:
https://github.com/llvm/llvm-project/issues/54469

going to close this Review now.

llvm/lib/Transforms/Scalar/Scalarizer.cpp

Scatterer::Scatterer(BasicBlock *bb, BasicBlock::iterator bbi, Value *v,
   if (Ty->isPointerTy()) {
     assert(cast<PointerType>(Ty)->isOpaqueOrPointeeTypeMatches(PtrElemTy) &&
            "Pointer element type mismatch");
     Ty = PtrElemTy;
   }
   Size = cast<FixedVectorType>(Ty)->getNumElements();
   if (!CachePtr)
     Tmp.resize(Size, nullptr);
-  else if (CachePtr->empty())
-    CachePtr->resize(Size, nullptr);
   else
-    assert(Size == CachePtr->size() && "Inconsistent vector sizes");
+    CachePtr->resize(Size, nullptr);
 }
 
 // Return component I, creating a new Value for it if necessary.
 Value *Scatterer::operator[](unsigned I) {
   ValueVector &CV = (CachePtr ? *CachePtr : Tmp);
   // Try to reuse a previous value.
   if (CV[I])
     return CV[I];