[PATCH] D93639: [AArch64][SVE]Add cost model for vector reduce for scalable vector

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Thank you @sdesmalen for helping to refine the code. 
I hope it is near to the end now.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1147-1148
+  switch (ISD) {
+  default:
+    return LegalizationCost + 16;
+  case ISD::UMIN:
----------------
sdesmalen wrote:
> I don't know what default could be here other than `llvm_unreachable`. And if that is unreachable, then you can just as well return the final cost on line 1137.
> 
> 
I've added unreachable to the switch for default as it is not expected to be reached at any time.
Without the default case the compiler complains:
//warning: control reaches end of non-void function//
as I did not added no return after the switch.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1189
+  case ISD::MUL:
+    return LegalizationCost + 16;
+  }
----------------
sdesmalen wrote:
> For Min/Max you use a fixed-cost of `1` and here you use `2` and `16`.  Does that mean Min/Max should also be 2?
> And should 16 actually be Invalid when this function will use InstructionCost because it cannot be expanded for SVE? If so, can you add a TODO?
So, I have changed the cost of MinMax and Arithmetic to be 2. 
I imagine it is fine for them to be the same. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93639/new/

https://reviews.llvm.org/D93639