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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:256
+
+  MachineOperand *DefOp0 = MRI.getOneDef(MI.getOperand(2).getReg());
+  MachineOperand *DefOp1 = MRI.getOneDef(MI.getOperand(3).getReg());
----------------
paquette wrote:
> In this case, you want to match
> 
> ```
> %op0 = G_TRUNC ...
> ...
> %op1 = G_TRUNC ...
> %.., %... = G_UADDO %op0, %op1
> ```
> 
> right?
> 
> I think you can do
> 
> ```
> if (!mi_match(MI.getOperand(2).getReg(), MRI, m_GTrunc(m_Reg(Op0Wide)) ||
>     !mi_match(MI.getOperand(3).getReg(), MRI, m_GTrunc(m_Reg(Op1Wide)))
>    return false;
> ```
yes, I merged them into a single if, thanks!


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:268
+  LLT OpTy = MRI.getType(ResVal);
+  MachineInstr *Op0WideDef = MRI.getOneDef(Op0Wide)->getParent();
+  MachineInstr *Op1WideDef = MRI.getOneDef(Op1Wide)->getParent();
----------------
paquette wrote:
> Any reason to not use `getVRegDef` here?
> 
> (I would recommend `getOpcodeDef`, but I think that walks past G_ASSERT_ZEXT)
> Any reason to not use getVRegDef here?

No reason I can think of. Updated!


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


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:305
+  // Remove G_ADDO.
+  B.setInstrAndDebugLoc(*MI.getNextNode());
+  Observer.erasingInstr(MI);
----------------
aemerson wrote:
> 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.
Happy to split this up/pull it in somewhere else if that's better. 


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:306
+  B.setInstrAndDebugLoc(*MI.getNextNode());
+  Observer.erasingInstr(MI);
+  MI.eraseFromParent();
----------------
paquette wrote:
> I don't think you need to tell the observer when you're deleting an instruction?
Thanks, looks like this can be dropped!


================
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);
----------------
aemerson wrote:
> 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`.
That makes things simpler, thanks! Did the same to get rid of `ResNew` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111888