[llvm] [LV] Create in-loop sub reductions (PR #147026)

[Sander de Smalen via llvm-commits]

================
@@ -897,8 +898,9 @@ RecurrenceDescriptor::InstDesc RecurrenceDescriptor::isRecurrenceInstr(
   case Instruction::PHI:
     return InstDesc(I, Prev.getRecKind(), Prev.getExactFPMathInst());
   case Instruction::Sub:
+    return InstDesc(Kind == RecurKind::Sub, I);
   case Instruction::Add:
-    return InstDesc(Kind == RecurKind::Add, I);
+    return InstDesc(Kind == RecurKind::Add || Kind == RecurKind::Sub, I);
----------------
sdesmalen-arm wrote:

It's more that the way this is written suggests asymmetrical behaviour, which wasn't asymmetrical before.

Before the code said "Finding a Sub or an Add instruction is allowed when matching RecurKind::Add", whereas now it says "Finding a Sub instruction is not allowed when matching RecurKind::Add", which is the functionality you actually want to support (and is required for those tests). However, this is now kind of supported by accident when matching a RecurKind::Sub.

Another way to view it is like this; in `isReductionPHI` it does:
```
if (AddReductionVar(Phi, RecurKind::Add, TheLoop, ...)) {
  LLVM_DEBUG(dbgs() << "Found an ADD reduction PHI." ...
  return true;
}
if (AddReductionVar(Phi, RecurKind::Sub, TheLoop, ...)) {
  LLVM_DEBUG(dbgs() << "Found an SUB reduction PHI." ...
  return true;
}
```

If this code were to be reversed by first trying to match `RecurKind::Sub` before matching `RecurKind::Add`, your code would return a different answer.

I think the right solution would be to do this:
```
case Instruction::Add:
  // A sub-reduction can handle 'add'
  if (Prev.getRecKind() == RecurKind::Sub && Kind == RecurKind::Sub)
    return InstDesc(I, Prev.getRecKind());
  else if (Kind == RecurKind::Add)
    return InstDesc(I, Kind);
  else
    return InstDesc(false, I);
```

and do a similar thing for `Instruction::Sub`.

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