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

[Danilo Carvalho Grael via Phabricator via llvm-commits]

dancgr marked an inline comment as not done.
dancgr added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2821
+  if (auto CNode = dyn_cast<ConstantSDNode>(N)) {
+    const int64_t ImmVal = CNode->getSExtValue();
+    SDLoc DL(N);
----------------
c-rhodes wrote:
> can this be `getZExtValue` now `i16` is being handled the same as `i32` and `i64`?
Yes, we don't need sext anymore. Will be updating this.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2828
+      Imm = CurDAG->getTargetConstant(ImmVal & 0xFF, DL, MVT::i32);
+      return true;
+    case MVT::i16:
----------------
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);
}
```


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:193
 def addsub_imm8_opt_lsl_i8  : imm8_opt_lsl<8,  "uint8_t",  SVEAddSubImmOperand8,  [{
   return AArch64_AM::isSVEAddSubImm<int8_t>(Imm);
 }]>;
----------------
Check for SVE AddSub Imm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71370