[PATCH] D144154: [X86]Use Class to refactor ArithMetic td file in X86

[Wang, Xin via Phabricator via llvm-commits]

XinWang10 added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrArithmetic.td:467
+class UnaryOpM<bits<8> opcode, Format f, string mnemonic, X86TypeInfo info,
+              list<dag> pattern>
+  : ITy<opcode, f, info, (outs), (ins info.MemOperand:$dst), mnemonic,
----------------
craig.topper wrote:
> Identation is off here
Hi, I'm not clear about the Identation rule here, could you please give me an example here?


================
Comment at: llvm/lib/Target/X86/X86InstrArithmetic.td:506
+  : UnaryOpR_C<opcode, AddRegFrm, mnemonic, info, []>, 
+                Requires<[Not64BitMode]>{
+  let Opcode = opcode;
----------------
craig.topper wrote:
> This Requires looks kind of funny. Might be better to switch to `let Predicates = [Not64BitMode]`
will do


================
Comment at: llvm/lib/Target/X86/X86InstrArithmetic.td:741
+let Defs = [AX,DX,EFLAGS], Uses = [AX] in
+def IMUL16r : MulOpR<0xF7, MRM5r, "imul", Xi16, WriteIMul16, []>;
+// EAX,EDX = EAX*GR32
----------------
XinWang10 wrote:
> skan wrote:
> > XinWang10 wrote:
> > > skan wrote:
> > > > Could we define `MulOpR` as a multiclass and use `defm IMUL`?
> > > I don't know if it is necessary? In my understanding, class and multiclass is designed for reuse, but if I rewrite a MulOpR as a multiclass here for IMUL, seems it has only one user here?
> > > 
> > What's difference? You already define class `MulOpR` for MUL and IMUL, and if we change it to multiclass, couldn't we use it for MUL and IMUL as well? And the suffix "8r", "16r", "32r", "64r" can be written only once.
> Because MUL8r and IMUL8r have different pattern.
I get your point, will take a try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144154