[PATCH] D78569: [SVE][CodeGen] Lower SDIV & UDIV to SVE intrinsics

Sander de Smalen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 23 03:45:24 PDT 2020


sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7670
+                     Mask, Op.getOperand(0), Op.getOperand(1));
+}
+
----------------
efriedma wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > If we're going to support these operations, we might as well just add isel patterns; that's what we've been doing for other arithmetic operations.
> > Just to provide a bit of context to this approach:
> > 
> > For unpredicated ISD nodes for which there is no predicated instruction, the predicate needs to be generated. For scalable vectors this will be a `ptrue all`, but for fixed-width vectors may take some other predicate such as VL8 for fixed `8` elements.
> > 
> > Rather than creating new predicated AArch64 ISD nodes for each operation such as `AArch64ISD::UDIV_PRED`, the idea is to reuse the intrinsic layer we already added to support the ACLE - which are predicated and for which we already have the patterns - and map directly onto those.
> > 
> > By doing the expansion in ISelLowering, the patterns stay simple and we can generalise `getPtrue` method so that it generates the right predicate for any scalable/fixed vector size as done in D71760 avoiding the need to write multiple patterns for different vector lengths.
> > 
> > This patch was meant as the proof of concept of that idea (as discussed in the sync-up call of Apr 2). 
> Using INTRINSIC_WO_CHAIN is a little annoying; it's hard to read in DAG dumps, and it gives weird error messages if we fail in selection.  But there aren't really any other immediate downsides I can think of, vs. doing it the other way (converting the intrinsic to AArch64ISD::UDIV_PRED).
> 
> Long-term, we're going to have a target-independent ISD::UDIV_PRED.  We probably want to start using those nodes at some point, to get target-independent optimizations. Not sure if that impacts what we want to do right now.
I agree that using INTRINSIC_WO_CHAIN will be a bit annoying for more complicated patterns. The reuse of the intrinsics was merely for convenience because we already have the patterns, and was not a critical part of the design. It shouldn't be a big effort to create AArch64ISD nodes and use these for the intrinsics as well.

If we use AArch64-specific nodes we can implement what's needed now for SVE codegen and I expect we can easily migrate to target-independent nodes when they get added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78569





More information about the llvm-commits mailing list