[llvm] [LV] Keep duplicate recipes in VPExpressionRecipe (PR #156976)

[Florian Hahn via llvm-commits]

https://github.com/fhahn commented:


> I agree with making the recipe flexible for future cases, but I don't see anything in this PR that would prevent it working with non-recipe extends. I think that's more of an issue with the constructor interface, so if we do end up making changes to accommodate for non-recipe extends then I don't think this PR will be making that work any harder. Pattern-matching the root instruction would certainly be a way to make the recipe very generic and flexible but it would of course introduce a lot of extra logic that we get for free right now.

I am not sure the pattern matching would introduce a lot of extra logic. Here's what I was thinking: we can get the multiply directly from the reduction recipe (since we need to look it up anyway). We know which operand it is and we can then get the casts from its operands. In this case, it work be even simpler, as we don't need to check `IsExtended` to get the multiply. 


```
@@ -2900,14 +2900,13 @@ void VPExpressionRecipe::print(raw_ostream &O, const Twine &Indent,
       << " (";
     O << "mul";
     bool IsExtended = ExpressionType == ExpressionTypes::ExtMulAccReduction;
-    auto *Mul = cast<VPWidenRecipe>(IsExtended ? ExpressionRecipes[2]
-                                               : ExpressionRecipes[0]);
+    auto *Mul = cast<VPWidenRecipe>(ExpressionRecipes.back()->getOperand(1));
     Mul->printFlags(O);
     if (IsExtended)
       O << "(";
     getOperand(0)->printAsOperand(O, SlotTracker);
     if (IsExtended) {
-      auto *Ext0 = cast<VPWidenCastRecipe>(ExpressionRecipes[0]);
+      auto *Ext0 = cast<VPWidenCastRecipe>(Mul->getOperand(0));
       O << " " << Instruction::getOpcodeName(Ext0->getOpcode()) << " to "
         << *Ext0->getResultType() << "), (";
     } else {
@@ -2915,7 +2914,7 @@ void VPExpressionRecipe::print(raw_ostream &O, const Twine &Indent,
     }
     getOperand(1)->printAsOperand(O, SlotTracker);
     if (IsExtended) {
-      auto *Ext1 = cast<VPWidenCastRecipe>(ExpressionRecipes[1]);
+      auto *Ext1 = cast<VPWidenCastRecipe>(Mul->getOperand(1));
       O << " " << Instruction::getOpcodeName(Ext1->getOpcode()) << " to "
         << *Ext1->getResultType() << ")";
     }
```

The above doesn't quite work  as expected at the moment unfortunately, because we currently end up with inconsistent operands for the VPExpressionRecipe if there are duplicates, which gets fixed by doing the replacement separately as in this patch. This change is quite subtle and the mapping of external operands to LiveInPlaceholders should probably be clarified/documented somewhere.


For the case in https://github.com/llvm/llvm-project/pull/147302/files#diff-34abe4c3cd34aa7a9664bbd204834248455635ba80b8a9ba9506d8c3e6b94d95R2878 we would need an extra check if the expression is negated, and depending on that use either the second-to-last or third-to-last recipe in the list.

As we need the operand remapping change in any case, the remaining changes seem minor. I still think the only reason they make some cases slighty more convenient is because we are missing support for extended reductions with constant operands, and once we support those we will need to look up the multiply w/o relying on the number of cast recipes anyways. But if you feel that it makes life easier, keeping duplicates is fine with me for now.

> EDIT: I tried rebasing on top of https://github.com/llvm/llvm-project/pull/161092 and all of the LV tests still pass.

That's because we don't create VPExpressionRecipes for partial reductions yet I think.


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