Page MenuHomePhabricator

[AArch64][SVE] Add integer arithmetic with immediate instructions.
ClosedPublic

Authored by dancgr on Dec 11 2019, 11:13 AM.

Details

Summary

Add pattern matching for the following instructions:

  • add, sub, subr, sqadd, sqsub, uqadd, uqsub

This patch required complex patterns to match the immediate with optinal left shift.

I re-used the Select function from the other SVE repo to implement the complext pattern.

I plan on doing another patch to also match constant vector of the same immediate.

Diff Detail

Event Timeline

dancgr created this revision.Dec 11 2019, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 11:13 AM

It's a little unfortunate you have to use ComplexPattern here, but I guess it's necessary given the way imm8_opt_lsl is represented.

llvm/include/llvm/IR/IntrinsicsAArch64.td
1050

Missing ImmArg marking. Second arg should be llvm_i32_ty.

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2833

Sign-extended where, exactly? I don't see any reason to allow @llvm.aarch64.sve.uqsub.imm.nxv8i16(<vscale x 8 x i16> %a, i32 -256)

dancgr marked 5 inline comments as done.Dec 11 2019, 2:23 PM
dancgr added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
1050

Updated that.

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2833

That makes sense. We don't have negative immediates for the AddSub, so it should be the same as i32 and i64. Will updated in the next patch.

dancgr updated this revision to Diff 233451.Dec 11 2019, 2:25 PM
dancgr marked 2 inline comments as done.
  • [AArch64][SVE] Update intrinsic to include Immediate option and remove sign handling for i16 immediate for add/sub.
dancgr updated this revision to Diff 233452.Dec 11 2019, 2:27 PM
  • [AArch64][SVE] Update intrinsic to include Immediate option and remove sign handling for i16 immediate for add/sub.
Harbormaster completed remote builds in B42344: Diff 233452.
efriedma added inline comments.Dec 11 2019, 3:31 PM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2829

I'd prefer to reject immediates greater than 255 here, instead of masking off the bits.

c-rhodes added inline comments.Dec 12 2019, 5:09 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2822

can this be getZExtValue now i16 is being handled the same as i32 and i64?

llvm/lib/Target/AArch64/SVEInstrFormats.td
296

SVE_1_Op_Imm_OptLsl_Pat

297

nit: formatting

llvm/test/CodeGen/AArch64/sve-int-imm.ll
9

nit: imm arg isn't directly under first arg, and below.

dancgr updated this revision to Diff 233617.Dec 12 2019, 7:39 AM
dancgr marked 7 inline comments as done.

Fix some formatting and apply suggested changes.

dancgr marked an inline comment as not done.Dec 12 2019, 7:43 AM
dancgr added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2822

Yes, we don't need sext anymore. Will be updating this.

2829

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);
}
llvm/lib/Target/AArch64/SVEInstrFormats.td
193

Check for SVE AddSub Imm

efriedma added inline comments.Dec 12 2019, 9:11 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2829

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.

dancgr updated this revision to Diff 233658.Dec 12 2019, 11:22 AM

Add extra check to DAGToDAGISel to avoid invalid immediates.

dancgr marked 4 inline comments as done.Dec 12 2019, 11:23 AM
dancgr added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2829

I was able to reproduce some instances where invalid immediates would get past that check. I have added that extra check.

efriedma added inline comments.Dec 12 2019, 12:20 PM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2829

Please put the "return true" inside the if statement.

2843

Missing "break" here. (It doesn't have any semantic effect, obviously, but you'll get a switch fallthrough on some compilers, I think.)

dancgr updated this revision to Diff 233670.Dec 12 2019, 12:45 PM
dancgr marked an inline comment as done.

Update minor details.

dancgr marked 2 inline comments as done.Dec 12 2019, 12:45 PM
This revision is now accepted and ready to land.Dec 12 2019, 1:28 PM
This revision was automatically updated to reflect the committed changes.