[PATCH] D71370: [AArch64][SVE] Add integer arithmetic with immediate instructions.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2828
+      Imm = CurDAG->getTargetConstant(ImmVal & 0xFF, DL, MVT::i32);
+      return true;
+    case MVT::i16:
----------------
dancgr wrote:
> efriedma wrote:
> > dancgr wrote:
> > > efriedma wrote:
> > > > I'd prefer to reject immediates greater than 255 here, instead of masking off the bits.
> > > In the immediate definition we have:
> > > 
> > > ```
> > > def addsub_imm8_opt_lsl_i8  : imm8_opt_lsl<8,  "uint8_t",  SVEAddSubImmOperand8,  [{
> > >   return AArch64_AM::isSVEAddSubImm<int8_t>(Imm);
> > > }]>;
> > > ```
> > > This checks if the immediate is in the accepted range for AddSub. Would you say we should also add another check here?
> > > 
> > > I pasted the code of that function below, since it is not in the changed files from this patch:
> > > 
> > > ```
> > > template <typename T>
> > > static inline bool isSVEAddSubImm(int64_t Imm) {
> > >   bool IsInt8t =
> > >       std::is_same<int8_t, typename std::make_signed<T>::type>::value;
> > >   return uint8_t(Imm) == Imm || (!IsInt8t && uint16_t(Imm & ~0xff) == Imm);
> > > }
> > > ```
> > Not sure that TableGen code is actually getting called from isel in this situation?  If it is, fine, we don't need a redundant check.
> I was able to reproduce some instances where invalid immediates would get past that check. I have added that extra check.
Please put the "return true" inside the if statement.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2842
+        return true;
+      }
+    default:
----------------
Missing "break" here.  (It doesn't have any semantic effect, obviously, but you'll get a switch fallthrough on some compilers, I think.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71370