[PATCH] D149731: [IR] New function llvm::createMinMaxSelectCmpOp for creating min/max operation in select-cmp form

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:943
 
+Value *llvm::createMinMaxSelectCmpOp(IRBuilderBase &Builder, RecurKind RK,
+                                     Value *Left, Value *Right) {
----------------
fhahn wrote:
> fhahn wrote:
> > Mel-Chen wrote:
> > > xbolva00 wrote:
> > > > nikic wrote:
> > > > > Mel-Chen wrote:
> > > > > > xbolva00 wrote:
> > > > > > > We have intrinsics for min and max. Do not emit cmp select form..
> > > > > > I am aware that we can use intrinsic to represent min max operations, but this patch aims to preserve the ability to express min max operations in select-cmp form. This is necessary for the vectorization feature I am currently developing. It is important to emphasize that this patch does not enforce the use of select-cmp form for min max operations, but rather provides an additional option.
> > > > > Why does your patch require the non-canonical select-cmp form? Please explain this in the patch description.
> > > > >> This is necessary for the vectorization feature I am currently developing.
> > > > 
> > > > Can you please share so we can check it and possibly suggest some tips?
> > > @nikic @xbolva00 Sure. Here is the WIP patch: [[ https://reviews.llvm.org/D143465 | D143465 ]]
> > > In short, we are developing a new reduction pattern, min max with index, for the vectorizer. In the function fixReduction process for interleaving, we need to generate the following IR in middle.block:
> > > 
> > > ```
> > > middle.block:                                     ; preds = %vector.body
> > >   ;; Start to fix minmax reduction
> > >   %rdx.minmax.cmp = icmp sgt i64 %12, %13
> > >   %rdx.minmax.select = select i1 %rdx.minmax.cmp, i64 %12, i64 %13
> > >   %rdx.minmax.cmp8 = icmp sgt i64 %rdx.minmax.select, %14
> > >   %rdx.minmax.select9 = select i1 %rdx.minmax.cmp8, i64 %rdx.minmax.select, i64 %14
> > >   %rdx.minmax.cmp10 = icmp sgt i64 %rdx.minmax.select9, %15
> > >   %rdx.minmax.select11 = select i1 %rdx.minmax.cmp10, i64 %rdx.minmax.select9, i64 %15
> > >    ;; Start to fix index reduction
> > >   %rdx.select = select i1 %rdx.minmax.cmp, i64 %20, i64 %21
> > >   %rdx.select12 = select i1 %rdx.minmax.cmp8, i64 %rdx.select, i64 %22
> > >   %rdx.select13 = select i1 %rdx.minmax.cmp10, i64 %rdx.select12, i64 %23
> > > ```
> > > For the handling of index reduction, we require the cmp part of the min max operation. This is the reason why I need this patch.
> > To clarify, is the sequence below could be expressed using intrinsics, 
> > ```
> >   %rdx.minmax.cmp = icmp sgt i64 %12, %13
> >   %rdx.minmax.select = select i1 %rdx.minmax.cmp, i64 %12, i64 %13
> >   %rdx.minmax.cmp8 = icmp sgt i64 %rdx.minmax.select, %14
> >   %rdx.minmax.select9 = select i1 %rdx.minmax.cmp8, i64 %rdx.minmax.select, i64 %14
> >   %rdx.minmax.cmp10 = icmp sgt i64 %rdx.minmax.select9, %15
> >   %rdx.minmax.select11 = select i1 %rdx.minmax.cmp10, i64 %rdx.minmax.select9, i64 %15
> > ```
> > 
> > but you would like to re-use the compares from above for the selcets below:
> > 
> > ```
> >    ;; Start to fix index reduction
> >   %rdx.select = select i1 %rdx.minmax.cmp, i64 %20, i64 %21
> >   %rdx.select12 = select i1 %rdx.minmax.cmp8, i64 %rdx.select, i64 %22
> >   %rdx.select13 = select i1 %rdx.minmax.cmp10, i64 %rdx.select12, i64 %23
> > ```
> > 
> > If that's the case it would probably be better to just keep the min/max select generation code local to the code that generates the 2nd chain of selects, instead of exposing this as general API
> > but you would like to re-use the compares from above for the selcets below:
> 
> @Mel-Chen is the comment above an accurate summary?
@fhahn 
Exactly, that's correct. I also believe there are better ways to address it, such as changing to generate the `CmpInst` during fixing index reduction, or generating an additional `CmpInst` during fixing min max reduction. Once I fix the issue you described, this patch will be abandoned. However, until it is fixed, we will temporarily rely on this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149731