[llvm-commits] [llvm] r120404 - in /llvm/trunk: lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h lib/Target/X86/X86InstrSSE.td test/CodeGen/X86/apm.ll

[Chris Lattner]

On Nov 29, 2010, at 11:20 PM, Eric Christopher wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=120404&view=rev
> Log:
> Rewrite mwait and monitor support and custom lower arguments.

Cool!

> +  
> +  // Address into RAX/EAX, other two args into ECX, EDX.
> +  unsigned MemOpc = Subtarget->is64Bit() ? X86::LEA64r : X86::LEA32r;
> +  unsigned MemReg = Subtarget->is64Bit() ? X86::RAX : X86::EAX;
> +  MachineInstrBuilder MIB = BuildMI(*BB, MI, dl, TII->get(MemOpc), MemReg);
> +  for (int i = 0; i < X86::AddrNumOperands; ++i)
> +    (*MIB).addOperand(MI->getOperand(i));

Please use -> instead of (*x).

> +MachineBasicBlock *
> +X86TargetLowering::EmitMwait(MachineInstr *MI, MachineBasicBlock *BB) const {
> +  assert((Subtarget->hasSSE3()) && "Target must have SSE3 features enabled");

No need for the redundant parens here.

> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.h Tue Nov 30 01:20:12 2010
> @@ -821,6 +821,13 @@
>     MachineBasicBlock *EmitPCMP(MachineInstr *BInstr, MachineBasicBlock *BB,
>                                 unsigned argNum, bool inMem) const;
> 
> +    /// Utility functions to emit monitor and mwait instructions. These
> +    /// need to make sure that the arguments to the intrinsic are in the
> +    /// correct registers.
> +    MachineBasicBlock *EmitMonitor(MachineInstr *MI, MachineBasicBlock *BB)
> +      const;

Minor and picky, but please wrap before 'MachineBasicBlock' so it isn't just a qualifier floating.

> +let usesCustomInserter = 1 in {
> +def MONITOR : I<0, Pseudo, (outs), (ins i32mem:$src1, GR32:$src2, GR32:$src3),
> +                "# MONITORrrr PSUEDO",
> +                [(int_x86_sse3_monitor addr:$src1, GR32:$src2, GR32:$src3)]>;
> +def MWAIT : I<0, Pseudo, (outs), (ins GR32:$src1, GR32:$src2),
> +                "# MWAITrr PSEUDO",
> +                [(int_x86_sse3_mwait GR32:$src1, GR32:$src2)]>;

Please use an empty asmstring to avoid bloating the asmprinter tables.  Better yet, X86 should use a PseudoI<> class that does the right thing.

Thanks for fixing this Eric!

-Chris