[PATCH] D46010: [AArch64] Improve cost of vector division by constant

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added a comment.

In https://reviews.llvm.org/D46010#1087835, @rengolin wrote:

> In https://reviews.llvm.org/D46010#1078477, @zatrazz wrote:
>
> > I should not change other architectures than AArch64 because 'isArithmeticDivFast' is a new method (only used by this patch).
>
>
> This still looks very AArch64-y to me. Those two ISD nodes are specific to the case you're working on. I'm sure there are a lot more "divs" than that. :)
>
> > but since the division by constant is a platform agnostic transformation I though it would make sense to check the same check on a platform agnostic manner.
>
> It is platform agnostic if you cover all cases that a platform agnostic constant divide function should. Right now, it's specific to this one issue in this one arch.


The divide-by-constant is lowered in a platform agnostic way at TargetLowering::BuildUDIV and TargetLowering::BuildSDIV (lib/CodeGen/SelectionDAG/TargetLowering.cpp). You can see that if target can lower either ISD::MULHS or ISD::SMUL_LOHI the multiply by inverse plus adjustment algorithm will be used. AArch64 is the case where it currently lowers ISD::MULHS.

What I am not sure if I need to handle the TargetTransformInfo::OP_PowerOf2 case, since DAGCombiner::visitSDIV will apply this transformation prior BuildSDIV/BuildUDIV.

> Alternatively, a simple static function here would be more appropriate. We can expand later if it proves to be more than just this case/arch.
> 
> I would mention that this is relying on the code generator to pick this exact pattern and hope nothing goes wrong between cost analysis and generation, but this is not new, so not for this patch. :)

Yes, ideally this kind of cost analysis would be better *after* the BuildUDIV/BuildSDIV expansion.


Repository:
  rL LLVM

https://reviews.llvm.org/D46010