[PATCH] D153164: [AArch64][GlobalISel] Select G_UADDE/G_SADDE/G_USUBE/G_SSUBE

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:4580
+  assert(LHS.isReg() && RHS.isReg() && "Expected register operands?");
+  MachineRegisterInfo &MRI = MIRBuilder.getMF().getRegInfo();
+  bool Is32Bit = (MRI.getType(LHS.getReg()).getSizeInBits() == 32);
----------------
`MachineIRBuilder` has `getMRI()`.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:4812-4813
+
+  bool OptAdd = false;
+  bool OptSub = false;
+  bool NegateCarry = false;
----------------
Since we only have 2 mutually exclusive choices, I'd just use something like `bool IsSub = false`. Then simplify below to:
```
if (Opcode == TargetOpcode::G_SSUBE || Opcode == TargetOpcode::G_USUBE)
  IsSub = true;
```
`NegateCarry` then is also redundant since it's only set when `IsSub = true`


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:4862-4881
+  switch (Opcode) {
+  default:
+    llvm_unreachable("Unexpected opcode!");
+  case TargetOpcode::G_SADDO:
+  case TargetOpcode::G_UADDO:
+  case TargetOpcode::G_SSUBO:
+  case TargetOpcode::G_USUBO:
----------------
This code looks pretty verbose. How about we add some new helper classes to `GenericMachineInstrs.h` so we have something like `GBinOpCarryOut`, `GBinOpCarryIn`. We could then eliminate this switch and replace with something like:
```
assert(isa<GBinOpCarryOut>(I) || isa<GBinOpCarryIn>(I));

if (auto *InMI = dyn_cast<GBinOpCarryIn>(&I)) {
  // Set NZCV carry according to carry-in VReg
  emitCarryIn(I, InMI->getCarryReg());
}
```
this way we also avoid hard coding an operand index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153164