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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:305
+  // Remove G_ADDO.
+  B.setInstrAndDebugLoc(*MI.getNextNode());
+  Observer.erasingInstr(MI);
----------------
paquette wrote:
> Typically we do the "actually combining the thing" part in a separate function.
> 
> (I'm not actually sure if it matters though...?)
Yeah, if we're bypassing the tablegen combiner I don't think we have to split it into separate functions.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:316-320
+  B.buildInstr(
+      TargetOpcode::G_AND, {CondBit},
+      {AddDst,
+       B.buildConstant(LLT::scalar(32),
+                       OpTy.getScalarSizeInBits() == 8 ? 1 << 8 : 1 << 16)});
----------------
use `buildAnd()`?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:321-324
+  Register CondCheck = MRI.cloneVirtualRegister(ResStatus);
+  B.buildICmp(CmpInst::ICMP_NE, CondCheck, CondBit,
+              B.buildConstant(LLT::scalar(32), 0));
+  Helper.replaceRegWith(MRI, ResStatus, CondCheck);
----------------
If we're going to replace ResStatus anyway, I think this can be written more succinctly IMO as:
```
B.buildICmp(CmpInst::ICMP_NE, ResStatus, CondBit, B.buildConstant(LLT::scalar(32), 0));
```
and then later delete `MI` which should take care of the original def of `ResStatus`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111888