This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add lowerings for {ADD,SUB}CARRY and S{ADD,SUB}O_CARRY
ClosedPublic

Authored by Kmeakin on Apr 7 2022, 10:08 AM.

Diff Detail

Event Timeline

Kmeakin created this revision.Apr 7 2022, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 10:08 AM
Kmeakin requested review of this revision.Apr 7 2022, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 10:08 AM

One nit plus a suggestion but otherwise the patch looks good to me. There are several regressions though, presumably caused by my request to split the original patch up. I'm not qualified to say how important these test cases are from a performance point of view but given the number of regressions I think it's worth creating a patch series (via Depends On) so the patches can be reviewed in isolation but pushed together. Sorry if this is messing you around.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3348

Unnecessary {} for single line body.

3352–3373

Up to you but I think these are better passed in a parameters so you end up with:

case ISD::ADDCARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, false /*unsigned*/);
case ISD::SUBCARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, false /*unsigned*/);
case ISD::SADDO_CARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, true /*signed*/);
case ISD::SSUBO_CARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, true /*signed*/);
dmgreen added a subscriber: dmgreen.

Yeah, we would need to make sure that the regressions are fixed either soon after, or preferably before this. 128 bit arithmetic is fairly rare, but can come up from certain disciplines. (Sometimes it's possible to test the optimizations independently with other code patterns. Sometimes that's not possible though).

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3348

Is this just checking for i32 or i64? It might be better to be explicit about that.
Also, I see this code is elsewhere too, but is it possible for types to be illegal here?

3375

OpLhs -> OpLHS

Kmeakin updated this revision to Diff 422807.Apr 14 2022, 4:13 AM

Update revision according to feedback

  • Remove unecessary {} for single-line body.
  • Pass in Opcode and IsSigned as parameters
  • Explicitly check for MVT::i32 and MVT::i64
  • Capitalisation of OpLHS and OpRHS
Kmeakin marked 4 inline comments as done.Apr 14 2022, 4:14 AM
Kmeakin updated this revision to Diff 422810.Apr 14 2022, 4:20 AM

Fix indentation

paulwalker-arm accepted this revision.Apr 21 2022, 5:15 AM
This revision is now accepted and ready to land.Apr 21 2022, 5:15 AM
This revision was landed with ongoing or failed builds.Apr 21 2022, 6:57 AM
This revision was automatically updated to reflect the committed changes.