[PATCH] D82377: [ARM] Allow tail predication on sadd_sat and uadd_sat intrinsics

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs marked 3 inline comments as done.
samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVETailPredication.cpp:356
       } else if (auto *Int = dyn_cast<IntrinsicInst>(&I)) {
-        if (Int->getIntrinsicID() == Intrinsic::fma)
+        if (Int->getIntrinsicID() == Intrinsic::fma || Int->getIntrinsicID() == Intrinsic::sadd_sat || Int->getIntrinsicID() == Intrinsic::uadd_sat)
           continue;
----------------
xbolva00 wrote:
> dmgreen wrote:
> > SjoerdMeijer wrote:
> > > Do we expect more intrinsics to be allowed after these 2?  For example, if we allow sadd, can we allow ssub too? And then while we are add it, add the ssubs too? 
> > > If we expect more intrinsics here, probably better to create an "AllowList" for intrinsics somewhere, and do a look-up here?
> > > 
> > Can you give this a clang-format? The line is a bit long.
> > 
> > We might end up with quite a few of these too. It might make sense to add a switch on Int->getIntrinsicID() once this gets large enough.
> +1 for switch
I could replace this with a switch statement as suggested. What do you think?


================
Comment at: llvm/lib/Target/ARM/MVETailPredication.cpp:356
       } else if (auto *Int = dyn_cast<IntrinsicInst>(&I)) {
-        if (Int->getIntrinsicID() == Intrinsic::fma)
+        if (Int->getIntrinsicID() == Intrinsic::fma || Int->getIntrinsicID() == Intrinsic::sadd_sat || Int->getIntrinsicID() == Intrinsic::uadd_sat)
           continue;
----------------
samtebbs wrote:
> xbolva00 wrote:
> > dmgreen wrote:
> > > SjoerdMeijer wrote:
> > > > Do we expect more intrinsics to be allowed after these 2?  For example, if we allow sadd, can we allow ssub too? And then while we are add it, add the ssubs too? 
> > > > If we expect more intrinsics here, probably better to create an "AllowList" for intrinsics somewhere, and do a look-up here?
> > > > 
> > > Can you give this a clang-format? The line is a bit long.
> > > 
> > > We might end up with quite a few of these too. It might make sense to add a switch on Int->getIntrinsicID() once this gets large enough.
> > +1 for switch
> I could replace this with a switch statement as suggested. What do you think?
Agreed. A switch would be much better.


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/tail-pred-intrinsic-uadd.ll:3
+; RUN: llc -mtriple=thumbv8.1m.main-none-none-eabi -mattr=+mve -verify-machineinstrs -disable-mve-tail-predication=false -o - %s | FileCheck %s
+define dso_local arm_aapcs_vfpcc void @arm_add_q15(i16* noalias nocapture readonly %pSrcA, i16* noalias nocapture readonly %pSrcB, i16* noalias nocapture %pDst, i32 %blockSize) local_unnamed_addr {
+; CHECK-LABEL: arm_add_q15:
----------------
dmgreen wrote:
> You can remove dso_local and local_unnamed_addr.
> 
> I would also make the two tests one file to keep them together, but up to you if you want to keep them separate.
I'll remove ds_local and local_unnamed and merge them +1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82377