[llvm-commits] [llvm] r50663 - in /llvm/trunk: include/llvm/ include/llvm/CodeGen/ include/llvm/Target/ lib/CodeGen/ lib/CodeGen/SelectionDAG/ lib/Target/ lib/Target/X86/ test/CodeGen/X86/

[Evan Cheng]

Hi Mon Ping,

Some comments.

On May 5, 2008, at 12:05 PM, Mon P Wang wrote:

>
> @@ -5732,6 +5744,187 @@
> //                           X86 Scheduler Hooks
> // 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
>
> +// private utility function
> +MachineBasicBlock *
> +X86TargetLowering::EmitAtomicBitwiseWithCustomInserter(MachineInstr  
> *bInstr,
> +                                                        
> MachineBasicBlock *MBB,
> +                                                       unsigned  
> regOpc,
> +                                                       unsigned  
> immOpc) {
> +  // For the atomic bitwise operator, we generate
> +  //   thisMBB:
> +  //   newMBB:
> +  //     ld  EAX = [bitinstr.addr]
> +  //     mov t1 = EAX
> +  //     op  t2 = t1, [bitinstr.val]

I think this is not quite right. lcs implicitly uses EAX so the value  
should be copied *to* EAX.  Also, only forcing the value into EAX just  
before its use is (slightly) better for register allocation.

t1 = ld [addr]
t2 = op t1, birinstr.val
eax = mov t1

>
> +  //     lcs dest = [bitinstr.addr], t2  [EAX is implicit]
> +  //     bz  newMBB
> +  //     fallthrough -->nextMBB
> +  const TargetInstrInfo *TII = getTargetMachine().getInstrInfo();
> +  const BasicBlock *LLVM_BB = MBB->getBasicBlock();
> +  ilist<MachineBasicBlock>::iterator MBBIter = MBB;
> +  ++MBBIter;
> +

Also, are you implementing llvm-gcc changes to enable it to generate  
these intrinsics?

Thanks,

Evan