Subzero. ARM32. Fix bugs uncovered by scons tests.

src/IceTargetLoweringARM32.cpp

 //===- subzero/src/IceTargetLoweringARM32.cpp - ARM32 lowering ------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 1397 matching lines @@
 //
 // This assumes Arg is an argument passed on the stack. This sets the frame
 // offset for Arg and updates InArgsSizeBytes according to Arg's width. For an
 // I64 arg that has been split into Lo and Hi components, it calls itself
 // recursively on the components, taking care to handle Lo first because of the
 // little-endian architecture. Lastly, this function generates an instruction
 // to copy Arg into its assigned register if applicable.
 void TargetARM32::finishArgumentLowering(Variable *Arg, Variable *FramePtr,
                                          size_t BasicFrameOffset,
                                          size_t *InArgsSizeBytes) {
+  const Type Ty = Arg->getType();
+  *InArgsSizeBytes = applyStackAlignmentTy(*InArgsSizeBytes, Ty);
+
   if (auto *Arg64On32 = llvm::dyn_cast<Variable64On32>(Arg)) {
-    Variable *Lo = Arg64On32->getLo();
-    Variable *Hi = Arg64On32->getHi();
+    Variable *const Lo = Arg64On32->getLo();
+    Variable *const Hi = Arg64On32->getHi();
     finishArgumentLowering(Lo, FramePtr, BasicFrameOffset, InArgsSizeBytes);
     finishArgumentLowering(Hi, FramePtr, BasicFrameOffset, InArgsSizeBytes);
     return;
   }
-  Type Ty = Arg->getType();
   assert(Ty != IceType_i64);
 
-  *InArgsSizeBytes = applyStackAlignmentTy(*InArgsSizeBytes, Ty);
   const int32_t ArgStackOffset = BasicFrameOffset + *InArgsSizeBytes;
   *InArgsSizeBytes += typeWidthInBytesOnStack(Ty);
 
   if (!Arg->hasReg()) {
     Arg->setStackOffset(ArgStackOffset);
     return;
   }
 
   // If the argument variable has been assigned a register, we need to copy the
   // value from the stack slot.
@@ skipping 3159 matching lines @@
   if (auto *Integer = llvm::dyn_cast<ConstantInteger32>(Opnd))
     return Integer->getValue();
   return Intrinsics::MemoryOrderInvalid;
 }
 } // end of anonymous namespace
 
 void TargetARM32::lowerLoadLinkedStoreExclusive(
     Type Ty, Operand *Addr, std::function<Variable *(Variable *)> Operation,
     CondARM32::Cond Cond) {
 
-  auto *Retry = Context.insert<InstARM32Label>(this);
+  InstARM32Label *Retry = nullptr;
+
+  Retry = Context.insert<InstARM32Label>(this);

[Eric Holk, 2016/03/07 22:58:53]

Why did you split this into two lines instead of leaving it as before?

[John, 2016/03/08 18:14:32]

Leftover from one of the changes.

+
   { // scoping for loop highlighting.
+    Variable *Success = makeReg(IceType_i32);
     Variable *Tmp = (Ty == IceType_i64) ? makeI64RegPair() : makeReg(Ty);
-    auto *Success = makeReg(IceType_i32);
     auto *_0 = Ctx->getConstantZero(IceType_i32);
 
     Context.insert<InstFakeDef>(Tmp);
     Context.insert<InstFakeUse>(Tmp);
     Variable *AddrR = legalizeToReg(Addr);
     _ldrex(Tmp, formMemoryOperand(AddrR, Ty))->setDestRedefined();
     auto *StoreValue = Operation(Tmp);
     assert(StoreValue->mustHaveReg());
-    _strex(Success, StoreValue, formMemoryOperand(AddrR, Ty), Cond);
-    _cmp(Success, _0, Cond);
+    // strex requires Dest to be a register other than Value or Addr. This
+    // restriction is cleanly represented by adding an "early" definition of
+    // Dest (or a latter use of all the sources.)
+    Context.insert<InstFakeDef>(Success);
+    if (Cond != CondARM32::AL) {
+      _mov_redefined(Success, legalize(_0, Legal_Reg | Legal_Flex),
+                     InstARM32::getOppositeCondition(Cond));
+    }
+    _strex(Success, StoreValue, formMemoryOperand(AddrR, Ty), Cond)
+        ->setDestRedefined();
+    _cmp(Success, _0);
   }
+
   _br(Retry, CondARM32::NE);
 }
 
 namespace {
 InstArithmetic *createArithInst(Cfg *Func, uint32_t Operation, Variable *Dest,
                                 Variable *Src0, Operand *Src1) {
   InstArithmetic::OpKind Oper;
   switch (Operation) {
   default:
     llvm::report_fatal_error("Unknown AtomicRMW operation");
@@ skipping 250 matching lines @@
     // We require the memory address to be naturally aligned. Given that is the
     // case, then normal loads are atomic.
     if (!Intrinsics::isMemoryOrderValid(
             ID, getConstantMemoryOrder(Instr->getArg(3)),
             getConstantMemoryOrder(Instr->getArg(4)))) {
       Func->setError("Unexpected memory ordering for AtomicCmpxchg");
       return;
     }
 
     if (DestTy == IceType_i64) {
+      Variable *LoadedValue = nullptr;
+      Variable **LoadedValuePtr = &LoadedValue;
+
       auto *New = makeI64RegPair();
       Context.insert<InstFakeDef>(New);
       lowerAssign(InstAssign::create(Func, New, Instr->getArg(2)));
 
       auto *Expected = makeI64RegPair();
       Context.insert<InstFakeDef>(Expected);
       lowerAssign(InstAssign::create(Func, Expected, Instr->getArg(1)));
 
       _dmb();
       lowerLoadLinkedStoreExclusive(
           DestTy, Instr->getArg(0),
-          [this, Expected, New, Instr, DestTy](Variable *Tmp) {
+          [this, Expected, New, Instr, DestTy, LoadedValuePtr](Variable *Tmp) {

[Eric Holk, 2016/03/07 22:58:53]

How do you feel about during a by-reference capture of LoadedValue instead of
adding LoadedValuePtr?

[John, 2016/03/08 18:14:32]

I don't like this idiom, but I dislike non const references even more. What I
really wanted here was a capture by pointer... :P

but done.

             auto *ExpectedLoR = llvm::cast<Variable>(loOperand(Expected));
             auto *ExpectedHiR = llvm::cast<Variable>(hiOperand(Expected));
             auto *TmpLoR = llvm::cast<Variable>(loOperand(Tmp));
             auto *TmpHiR = llvm::cast<Variable>(hiOperand(Tmp));
             _cmp(TmpLoR, ExpectedLoR);
             _cmp(TmpHiR, ExpectedHiR, CondARM32::EQ);
+            *LoadedValuePtr = Tmp;
             // Adding an explicit use of Tmp here, or its live range will not
             // reach here (only those of Tmp.Lo and Tmp.Hi will.)
-            Context.insert<InstFakeUse>(Tmp);
-            _mov_redefined(ExpectedLoR, TmpLoR);
-            _mov_redefined(ExpectedHiR, TmpHiR);
+            // Context.insert<InstFakeUse>(Tmp);

[Jim Stichnoth, 2016/03/07 23:25:48]

Delete these comments?

[John, 2016/03/08 18:14:32]

Done.

+            //_mov_redefined(ExpectedLoR, TmpLoR);
+            //_mov_redefined(ExpectedHiR, TmpHiR);
             // Same as above.
-            Context.insert<InstFakeUse>(Tmp);
+            // Context.insert<InstFakeUse>(Tmp);
             return New;
           },
           CondARM32::EQ);
       _dmb();
 
-      lowerAssign(InstAssign::create(Func, Dest, Expected));
+      Context.insert<InstFakeUse>(LoadedValue);
+      lowerAssign(InstAssign::create(Func, Dest, LoadedValue));
       // The fake-use Expected prevents the assignments to Expected (above)
       // from being removed if Dest is not used.
       Context.insert<InstFakeUse>(Expected);
       // New needs to be alive here, or its live range will end in the
       // strex instruction.
       Context.insert<InstFakeUse>(New);
       return;
     }
 
     auto *New = legalizeToReg(Instr->getArg(2));
     auto *Expected = legalizeToReg(Instr->getArg(1));
+    Variable *LoadedValue = nullptr;
+    Variable **LoadedValuePtr = &LoadedValue;
 
     _dmb();
     lowerLoadLinkedStoreExclusive(
-        DestTy,
-        Instr->getArg(0), [this, Expected, New, Instr, DestTy](Variable *Tmp) {
+        DestTy, Instr->getArg(0),
+        [this, Expected, New, Instr, DestTy, LoadedValuePtr](Variable *Tmp) {
           lowerIcmpCond(InstIcmp::Eq, Tmp, Expected);
-          _mov_redefined(Expected, Tmp);
+          *LoadedValuePtr = Tmp;
+          //_mov_redefined(Expected, Tmp);
           return New;
-        }, CondARM32::EQ);
+        },
+        CondARM32::EQ);
     _dmb();
 
-    lowerAssign(InstAssign::create(Func, Dest, Expected));
+    lowerAssign(InstAssign::create(Func, Dest, LoadedValue));
     Context.insert<InstFakeUse>(Expected);
     Context.insert<InstFakeUse>(New);
     return;
   }
   case Intrinsics::AtomicRMW: {
     if (!Intrinsics::isMemoryOrderValid(
             ID, getConstantMemoryOrder(Instr->getArg(3)))) {
       Func->setError("Unexpected memory ordering for AtomicRMW");
       return;
     }
@@ skipping 1928 matching lines @@
   // However, for compatibility with current NaCl LLVM, don't claim that.
   Str << ".eabi_attribute 14, 3   @ Tag_ABI_PCS_R9_use: Not used\n";
 }
 
 SmallBitVector TargetARM32::TypeToRegisterSet[RegARM32::RCARM32_NUM];
 SmallBitVector TargetARM32::TypeToRegisterSetUnfiltered[RegARM32::RCARM32_NUM];
 SmallBitVector TargetARM32::RegisterAliases[RegARM32::Reg_NUM];
 
 } // end of namespace ARM32
 } // end of namespace Ice