[llvm-commits] [PATCH] Atomic NAND options

[Chris Lattner]

On Jul 14, 2011, at 9:44 AM, Cameron McInally wrote:

> Well... it is a change of semantics. Users who expect the intrinsic to
> work the old way will be surprised when tests begin to fail. But, in
> my opinion, there's no reason to keep the old implementation.
> 
> Perhaps a warning should be issued to inform the user that the
> underlying implementation has changed. That's what GCC has done to
> address this issue.

Yes, I think that is very reasonable.  It is a very rare builtin for people to use.  Having clang warn unconditionally (with a -W flag) seems like the right answer.

-Chris

> 
> On Thursday, July 14, 2011, Chris Lattner <clattner at apple.com> wrote:
>> 
>> On Jul 14, 2011, at 8:03 AM, Cameron McInally wrote:
>> Hey Guys,
>> 
>> I have been working on Atomic NAND. Here is a patch that allows the
>> compiler writer to select a [NOT AND] implementation, while retaining
>> [NEGATE and AND] as the default implementation for those that do not
>> want the current GCC implementation (i.e. GCC v4.4 and later).
>> 
>> Hi Cameron,
>> Is there a reason to support the broken pre-gcc-4.4 implementation at all?
>> -Chris
>> 
>> upstream/llvm> svn diff
>> Index: lib/Target/X86/X86ISelLowering.cpp
>> ===================================================================
>> --- lib/Target/X86/X86ISelLowering.cpp    (revision 135054)
>> +++ lib/Target/X86/X86ISelLowering.cpp    (working copy)
>> @@ -9710,7 +9710,8 @@
>>                                                         unsigned notOpc,
>>                                                         unsigned EAXreg,
>> 
>> TargetRegisterClass *RC,
>> -                                                       bool invSrc) const {
>> +                                                       bool invSrc,
>> +                                bool invRes) const {
>>    // For the atomic bitwise operator, we generate
>>    //   thisMBB:
>>    //   newMBB:
>> @@ -9783,13 +9784,20 @@
>>    MIB.addReg(tt);
>>    (*MIB).addOperand(*argOpers[valArgIndx]);
>> 
>> +  unsigned tr = F->getRegInfo().createVirtualRegister(RC);
>> +  if (invRes) {
>> +    MIB = BuildMI(newMBB, dl, TII->get(notOpc), tr).addReg(t2);
>> +  }
>> +  else
>> +    tr = t2;
>> +
>>    MIB = BuildMI(newMBB, dl, TII->get(TargetOpcode::COPY), EAXreg);
>>    MIB.addReg(t1);
>> 
>>    MIB = BuildMI(newMBB, dl, TII->get(CXchgOpc));
>>    for (int i=0; i <= lastAddrIndx; ++i)
>>      (*MIB).addOperand(*argOpers[i]);
>> -  MIB.addReg(t2);
>> +  MIB.addReg(tr);
>>    assert(bInstr->hasOneMemOperand() && "Unexpected number of memoperand");
>>    (*MIB).setMemRefs(bInstr->memoperands_begin(),
>>                      bInstr->memoperands_end());
>> @@ -9812,7 +9820,8 @@
>>                                                         unsigned regOpcH,
>>                                                         unsigned immOpcL,
>>                                                         unsigned immOpcH,
>> -                                                       bool invSrc) const {
>> +                                                       bool invSrc,
>> +                                bool invRes) const {
>>    // For the atomic bitwise operator, we generate
>>    //   thisMBB (instructions are in pairs, except cmpxchg8b)
>>    //     ld t1,t2 = [bitinstr.addr]
>> @@ -9939,15 +9948,26 @@
>>      MIB.addReg(t2);
>>    (*MIB).addOperand(*argOpers[valArgIndx + 1]);
>> 
>> +  unsigned trl = F->getRegInfo().createVirtualRegister(RC);
>> +  unsigned trh = F->getRegInfo().createVirtualRegister(RC);
>> +  if (invRes) {
>> +    MIB = BuildMI(newMBB, dl, TII->get(NotOpc), trl).addReg(t5);
>> +    MIB = BuildMI(newMBB, dl, TII->get(NotOpc), trh).addReg(t6);
>> +  }
>> +  else {
>> +    trl = t5;
>> +    trh = t6;
>> +  }
>> +
>>    MIB = BuildMI(newMBB, dl, TII->get(TargetOpcode::COPY), X86::EAX);
>>    MIB.addReg(t1);
>>    MIB = BuildMI(newMBB, dl, TII->get(TargetOpcode::COPY), X86::EDX);
>>    MIB.addReg(t2);
>> 
>>    MIB = BuildMI(newMBB, dl, TII->get(TargetOpcode::COPY), X86::EBX);
>> -  MIB.addReg(t5);
>> +  MIB.addReg(trl);
>>    MIB = BuildMI(newMBB, dl, TII->get(TargetOpcode::COPY), X86::ECX);
>> -  MIB.addReg(t6);
>> +  MIB.addReg(trh);
>> 
>>    MIB = BuildMI(newMBB, dl, TII->get(X86::LCMPXCHG8B));
>>    for (int i=0; i <= lastAddrIndx; ++i)
>> Index: lib/Target/X86/X86ISelLowering.h
>> ===================================================================
>> --- lib/Target/X86/X86ISelLowering.h    (revision 135054)
>> +++ lib/Target/X86/X86ISelLowering.h    (working copy)
>> @@ -896,7 +896,8 @@
>>                                                      unsigned notOpc,
>>                                                      unsigned EAXreg,
>>                                                      TargetRegisterClass
>> *RC,
>> -                                                    bool invSrc =
>> false) const;
>> +                                                    bool invSrc = false,
>> +                             bool invRes = false) const;
>> 
>>      MachineBasicBlock *EmitAtomicBit6432WithCustomInserter(
>>                                                      MachineInstr *BInstr,
>> @@ -905,7 +906,8 @@
>>                                                      unsigned regOpcH,
>>                                                      unsigned immOpcL,
>>                                                      unsigned immOpcH,
>> -                                                    bool invSrc =
>> false) const;
>> +                                                    bool invSrc = false,
>> +                             bool invRes = false) const;
>> 
>>      /// Utility function to emit atomic min and max.  It takes the min/max
>>      /// instruction to expand, the associated basic block, and the
>> associated
>> 
>> 
>> With the above patch, the current LLVM implementation will be selected.
>> To enablCameron
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>>