[llvm] LV: Expand llvm.histogram intrinsic to support umax, umin, and uadd.sat operations (PR #127399)
via llvm-commits
llvm-commits at lists.llvm.org
Sun May 4 03:43:15 PDT 2025
RonDahan101 wrote:
> > It would be good to add the intrinsics together with initial lowering as separate patch, including codegen tests.
>
> It would still be good to separate the changes for adding the new intrinsics from using them in LV. Also the description/titl needs updating.
> > > > It would be good to add the intrinsics together with initial lowering as separate patch, including codegen tests.
> > >
> > >
> > > It would still be good to separate the changes for adding the new intrinsics from using them in LV. Also the description/titl needs updating.
> >
> >
> > I updated the title and description. I'm not sure I understand what you're proposing. Adding the intrinsics without using them in LV seems redundant. What's the point of adding unused intrinsics? If you meant something else, please elaborate on what's needs to be done.
> > Thanks in advance for taking your time and reviewing.
>
> Splitting it into separate patches is often done to make it easier to review or limit the amount of code that must be examined if a bug is found later in a given commit. I did create a separate patch for the initial histogram intrinsic in [fbb37e9](https://github.com/llvm/llvm-project/commit/fbb37e960616efcf7cd5c1ebbe95f75c65d565dc), along with AArch64 codegen. The use in LoopVectorize came later in [6f1a8c2](https://github.com/llvm/llvm-project/commit/6f1a8c2da278a04565877e277bc4d5b70055ac74).
Thank you both for your patience and explanation. I have uploaded a new PR that adds the intrinsic without utilizing it in the LV. Once the previous patch is merged, I will update the current PR to include only the LV changes and notify you.
New PR: https://github.com/llvm/llvm-project/pull/138447
Thank you for your time.
https://github.com/llvm/llvm-project/pull/127399
More information about the llvm-commits
mailing list