[llvm] [LV] Strengthen calls to collectInstsToScalarize (NFC) (PR #130642)

[David Sherwood via llvm-commits]

================
@@ -5393,20 +5393,21 @@ bool LoopVectorizationCostModel::useEmulatedMaskMemRefHack(Instruction *I,
 }
 
 void LoopVectorizationCostModel::collectInstsToScalarize(ElementCount VF) {
-  // If we aren't vectorizing the loop, or if we've already collected the
-  // instructions to scalarize, there's nothing to do. Collection may already
-  // have occurred if we have a user-selected VF and are now computing the
-  // expected cost for interleaving.
-  if (VF.isScalar() || VF.isZero() || InstsToScalarize.contains(VF))
+  assert(VF.isVector() && "Expected VF >= 2");
+
+  // If we've already collected the instructions to scalarize or the predicated
+  // BBs after vectorization, there's nothing to do. Collection may already have
+  // occurred if we have a user-selected VF and are now computing the expected
+  // cost for interleaving.
+  if (InstsToScalarize.contains(VF) ||
+      PredicatedBBsAfterVectorization.contains(VF))
     return;
 
   // Initialize a mapping for VF in InstsToScalalarize. If we find that it's
   // not profitable to scalarize any instructions, the presence of VF in the
   // map will indicate that we've analyzed it already.
   ScalarCostsTy &ScalarCostsVF = InstsToScalarize[VF];
 
-  PredicatedBBsAfterVectorization[VF].clear();
----------------
david-arm wrote:

Yes, I was precisely thinking it might be a correctness issue. Before your change we unconditionally populate `PredicatedBBsAfterVectorization` with an entry for a VF, even if all we do is create an empty entry. I think this means that `contains(VF)` would always return true. Whereas after your patch we only populate `PredicatedBBsAfterVectorization` if we find an instruction that is scalar with predication, therefore `contains(VF)` may or may not return true. In practice it may not be an issue, but I just wanted to confirm this.

https://github.com/llvm/llvm-project/pull/130642