[PATCH] D111888: [AArch64][GISel] Optimize 8 and 16 bit variants of uaddo.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:254
+
+  auto &MRI = Helper.getMRI();
+
----------------
You can get MRI from the builder.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:274
+      Op1WideDef->getOpcode() != TargetOpcode::G_ASSERT_ZEXT ||
+      OpTy.getScalarSizeInBits() != Op0WideDef->getOperand(2).getImm() ||
+      OpTy.getScalarSizeInBits() != Op1WideDef->getOperand(2).getImm())
----------------
Pull `OpTy.getScalarSizeInBits()` into a variable?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:279
+  // Only scalar UADDO with either 8 or 16 bit operands are handled.
+  if (!WideTy0.isScalar() || !WideTy1.isScalar() || WideTy0 != WideTy1 ||
+      OpTy.getScalarSizeInBits() >= WideTy0.getScalarSizeInBits() ||
----------------
I think the `!WideTy1.isScalar() ` check is redundant


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:286
+  Register ResStatus = MI.getOperand(1).getReg();
+  if (!Helper.getMRI().hasOneUse(ResStatus))
+    return false;
----------------
- Reuse MRI variable
- Avoid debug value weirdness


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:305
+  // Remove G_ADDO.
+  B.setInstrAndDebugLoc(*MI.getNextNode());
+  Observer.erasingInstr(MI);
----------------
Typically we do the "actually combining the thing" part in a separate function.

(I'm not actually sure if it matters though...?)


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:306
+  B.setInstrAndDebugLoc(*MI.getNextNode());
+  Observer.erasingInstr(MI);
+  MI.eraseFromParent();
----------------
I don't think you need to tell the observer when you're deleting an instruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111888