Avoid passing objects with __declspec(align) members by value (PR24113) - llvm part

[hans]

Summary

This allows us to remove the specializations of AlignedCharArray<8> etc. that tried to use e.g. a union with 'double' to get alignment while avoiding the declspec. That work-around never really worked, as those unions wouldn't cause stack realignment, and an AlignedCharArray<8> on the stack would not necessarily be aligned.

[rnk]

include/llvm/ADT/iterator_range.h

35

I'd bug Chandler about this.

include/llvm/IR/PassManagerInternal.h

313

I'd bug Chandler about this.

include/llvm/Support/AlignOf.h

140–142

So, it occurs to me that MSVC can guarantee 1, 2, or 4 byte stack alignment on x86, just not 8 or more. Does it help reduce the diff if we undo these?

lib/CodeGen/AsmPrinter/DIEHash.h

137

For DIEValue, we could ask Duncan why he used AlignedCharArrayUnion instead of regular union in the first place.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

192

The whole ARM asm parser probably didn't meant to pass these by value and can probably be committed as is.

[hans]

I've worked around the PR by allocating the IntervalMap in ELF.h on the heap instead, r242157.

I probably won't pursue this patch further right now.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

192

r242160

include/llvm/ADT/iterator_range.h

 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
 /// fancy at all.
 template <typename IteratorT>
 class iterator_range {
   IteratorT begin_iterator, end_iterator;
 
 public:
-  iterator_range(IteratorT begin_iterator, IteratorT end_iterator)
-      : begin_iterator(std::move(begin_iterator)),
-        end_iterator(std::move(end_iterator)) {}
+  iterator_range(const IteratorT &begin_iterator, const IteratorT &end_iterator)
+      : begin_iterator(begin_iterator),
+        end_iterator(end_iterator) {}
 
   IteratorT begin() const { return begin_iterator; }
   IteratorT end() const { return end_iterator; }
 };
 
 /// \brief Convenience function for iterating over sub-ranges.
 ///
 /// This provides a bit of syntactic sugar to make using sub-ranges
 /// in for loops a bit easier. Analogous to std::make_pair().
-template <class T> iterator_range<T> make_range(T x, T y) {
-  return iterator_range<T>(std::move(x), std::move(y));
+template <class T> iterator_range<T> make_range(const T &x, const T &y) {
+  return iterator_range<T>(x, y);
 }
 
 template <typename T> iterator_range<T> make_range(std::pair<T, T> p) {
-  return iterator_range<T>(std::move(p.first), std::move(p.second));
+  return iterator_range<T>(p.first, p.second);
 }
 }
 
 #endif

include/llvm/CodeGen/DIE.h

 ///     linked list here makes this way easier to accomplish.
 ///  2. Carrying an extra pointer per \a DIEValue isn't expensive.  45% of DIEs
 ///     have 2 or fewer values, and 90% have 5 or fewer.  A vector would be
 ///     over-allocated by 50% on average anyway, the same cost as the
 ///     linked-list node.
 class DIEValueList {
   struct Node : IntrusiveBackListNode {
     DIEValue V;
-    explicit Node(DIEValue V) : V(V) {}
+    explicit Node(const DIEValue &V) : V(V) {}
   };
 
   typedef IntrusiveBackList<Node> ListTy;
   ListTy List;
 
 public:
   bool empty() const { return List.empty(); }
 
public:
     const_iterator() = default;
     const_iterator(DIEValueList::iterator X) : iterator_adaptor(X.wrapped()) {}
     explicit const_iterator(ListTy::const_iterator X) : iterator_adaptor(X) {}
 
     explicit operator bool() const { return bool(wrapped()); }
     const DIEValue &operator*() const { return wrapped()->V; }
   };
 
-  iterator insert(BumpPtrAllocator &Alloc, DIEValue V) {
+  iterator insert(BumpPtrAllocator &Alloc, const DIEValue &V) {
     List.push_back(*new (Alloc) Node(V));
     return iterator(ListTy::toIterator(List.back()));
   }
   template <class... Ts>
   iterator emplace(BumpPtrAllocator &Alloc, Ts &&... Args) {
     return insert(Alloc, DIEValue(std::forward<Ts>(Args)...));
   }
 
public:
   /// Similar to getUnit, returns null when DIE is not added to an
   /// owner yet.
   const DIE *getUnitOrNull() const;
   void setOffset(unsigned O) { Offset = O; }
   void setSize(unsigned S) { Size = S; }
 
   /// addValue - Add a value and attributes to a DIE.
   ///
-  value_iterator addValue(BumpPtrAllocator &Alloc, DIEValue Value) {
+  value_iterator addValue(BumpPtrAllocator &Alloc, const DIEValue &Value) {
     return Values.insert(Alloc, Value);
   }
   template <class T>
   value_iterator addValue(BumpPtrAllocator &Alloc, dwarf::Attribute Attribute,
                           dwarf::Form Form, T &&Value) {
     return Values.emplace(Alloc, Attribute, Form, std::forward<T>(Value));
   }
 

include/llvm/IR/PassManager.h

typename PassT::Result *getCachedResult(IRUnitT &IR) const {
     return &static_cast<ResultModelT *>(ResultConcept)->Result;
   }
 
   /// \brief Register an analysis pass with the manager.
   ///
   /// This provides an initialized and set-up analysis pass to the analysis
   /// manager. Whomever is setting up analysis passes must use this to populate
   /// the manager with all of the analysis passes available.
-  template <typename PassT> void registerPass(PassT Pass) {
+  template <typename PassT> void registerPass(const PassT &Pass) {
     assert(!AnalysisPasses.count(PassT::ID()) &&
            "Registered the same analysis pass twice!");
     typedef detail::AnalysisPassModel<IRUnitT, PassT> PassModelT;
-    AnalysisPasses[PassT::ID()].reset(new PassModelT(std::move(Pass)));
+    AnalysisPasses[PassT::ID()].reset(new PassModelT(Pass));
   }
 
   /// \brief Invalidate a specific analysis pass for an IR module.
   ///
   /// Note that the analysis result can disregard invalidation.
   template <typename PassT> void invalidate(IRUnitT &IR) {
     assert(AnalysisPasses.count(PassT::ID()) &&
            "This analysis pass was not registered prior to being invalidated");

include/llvm/IR/PassManagerInternal.h

struct AnalysisPassModel<IRUnitT, PassT, true> : AnalysisPassConcept<IRUnitT> {
 
   PassT Pass;
 };
 
 /// \brief Specialization of \c AnalysisPassModel which does not pass an
 /// \c AnalysisManager to PassT's run method.
 template <typename IRUnitT, typename PassT>
 struct AnalysisPassModel<IRUnitT, PassT, false> : AnalysisPassConcept<IRUnitT> {
-  explicit AnalysisPassModel(PassT Pass) : Pass(std::move(Pass)) {}
+  explicit AnalysisPassModel(const PassT &Pass) : Pass(Pass) {}
   // We have to explicitly define all the special member functions because MSVC
   // refuses to generate them.
   AnalysisPassModel(const AnalysisPassModel &Arg) : Pass(Arg.Pass) {}
   AnalysisPassModel(AnalysisPassModel &&Arg) : Pass(std::move(Arg.Pass)) {}
   friend void swap(AnalysisPassModel &LHS, AnalysisPassModel &RHS) {
     using std::swap;
     swap(LHS.Pass, RHS.Pass);
   }

include/llvm/Object/SymbolicFile.h

 }
 
 template <class content_type>
 class content_iterator
     : public std::iterator<std::forward_iterator_tag, content_type> {
   content_type Current;
 
 public:
-  content_iterator(content_type symb) : Current(symb) {}
+  content_iterator(const content_type &symb) : Current(symb) {}
 
   const content_type *operator->() const { return &Current; }
 
   const content_type &operator*() const { return Current; }
 
   bool operator==(const content_iterator &other) const {
     return Current == other.Current;
   }

include/llvm/Support/AlignOf.h

 #endif
 
 #else // _MSC_VER
 
 /// \brief Create a type with an aligned char buffer.
 template<std::size_t Alignment, std::size_t Size>
 struct AlignedCharArray;
 
-// We provide special variations of this template for the most common
-// alignments because __declspec(align(...)) doesn't actually work when it is
-// a member of a by-value function argument in MSVC, even if the alignment
-// request is something reasonably like 8-byte or 16-byte. Note that we can't
-// even include the declspec with the union that forces the alignment because
-// MSVC warns on the existence of the declspec despite the union member forcing
-// proper alignment.
-
-template<std::size_t Size>
-struct AlignedCharArray<1, Size> {
-  union {
-    char aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<2, Size> {
-  union {
-    short aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<4, Size> {
-  union {
-    int aligned;
-    char buffer[Size];
-  };
-};
-
-template<std::size_t Size>
-struct AlignedCharArray<8, Size> {
-  union {
-    double aligned;
-    char buffer[Size];
-  };
-};
-
-
-// The rest of these are provided with a __declspec(align(...)) and we simply
-// can't pass them by-value as function arguments on MSVC.
+// Note: these can't be passed by value because of the __declspec(align(..)).
 
 #define LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(x) \
   template<std::size_t Size> \
   struct AlignedCharArray<x, Size> { \
     __declspec(align(x)) char buffer[Size]; \
   };
 
+LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(1)
+LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(2)
+LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(4)
+LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(8)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(16)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(32)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(64)
 LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(128)
 
 #undef LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT
 
 #endif // _MSC_VER

lib/CodeGen/AsmPrinter/DIEHash.h

private:
   /// \brief Hashes the data in a block like DIEValue, e.g. DW_FORM_block or
   /// DW_FORM_exprloc.
   void hashBlockData(const DIE::const_value_range &Values);
 
   /// \brief Hashes the contents pointed to in the .debug_loc section.
   void hashLocList(const DIELocList &LocList);
 
   /// \brief Hashes an individual attribute.
-  void hashAttribute(DIEValue Value, dwarf::Tag Tag);
+  void hashAttribute(const DIEValue &Value, dwarf::Tag Tag);
 
   /// \brief Hashes an attribute that refers to another DIE.
   void hashDIEEntry(dwarf::Attribute Attribute, dwarf::Tag Tag,
                     const DIE &Entry);
 
   /// \brief Hashes a reference to a named type in such a way that is
   /// independent of whether that type is described by a declaration or a
   /// definition.

lib/CodeGen/AsmPrinter/DIEHash.cpp

void DIEHash::hashLocList(const DIELocList &LocList) {
   DwarfDebug &DD = *AP->getDwarfDebug();
   const DebugLocStream &Locs = DD.getDebugLocs();
   for (const auto &Entry : Locs.getEntries(Locs.getList(LocList.getValue())))
     DD.emitDebugLocEntry(Streamer, Entry);
 }
 
 // Hash an individual attribute \param Attr based on the type of attribute and
 // the form.
-void DIEHash::hashAttribute(DIEValue Value, dwarf::Tag Tag) {
+void DIEHash::hashAttribute(const DIEValue &Value, dwarf::Tag Tag) {
   dwarf::Attribute Attribute = Value.getAttribute();
 
   // Other attribute values use the letter 'A' as the marker, and the value
   // consists of the form code (encoded as an unsigned LEB128 value) followed by
   // the encoding of the value according to the form code. To ensure
   // reproducibility of the signature, the set of forms used in the signature
   // computation is limited to the following: DW_FORM_sdata, DW_FORM_flag,
   // DW_FORM_string, and DW_FORM_block.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

bool Warning(SMLoc L, const Twine &Msg,
                ArrayRef<SMRange> Ranges = None) {
     return getParser().Warning(L, Msg, Ranges);
   }
   bool Error(SMLoc L, const Twine &Msg,
              ArrayRef<SMRange> Ranges = None) {
     return getParser().Error(L, Msg, Ranges);
   }
 
-  bool validatetLDMRegList(MCInst Inst, const OperandVector &Operands,
+  bool validatetLDMRegList(const MCInst &Inst, const OperandVector &Operands,
                            unsigned ListNo, bool IsARPop = false);
-  bool validatetSTMRegList(MCInst Inst, const OperandVector &Operands,
+  bool validatetSTMRegList(const MCInst &Inst, const OperandVector &Operands,
                            unsigned ListNo);
 
   int tryParseRegister();
   bool tryParseRegisterWithWriteBack(OperandVector &);
   int tryParseShiftRegister(OperandVector &);
   bool parseRegisterList(OperandVector &);
   bool parseMemory(OperandVector &);
   bool parseOperand(OperandVector &, StringRef Mnemonic);
bool ARMAsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
   return false;
 }
 
 // Validate context-sensitive operand constraints.
 
 // return 'true' if register list contains non-low GPR registers,
 // 'false' otherwise. If Reg is in the register list or is HiReg, set
 // 'containsReg' to true.
-static bool checkLowRegisterList(MCInst Inst, unsigned OpNo, unsigned Reg,
-                                 unsigned HiReg, bool &containsReg) {
+static bool checkLowRegisterList(const MCInst &Inst, unsigned OpNo,
+                                 unsigned Reg, unsigned HiReg,
+                                 bool &containsReg) {
   containsReg = false;
   for (unsigned i = OpNo; i < Inst.getNumOperands(); ++i) {
     unsigned OpReg = Inst.getOperand(i).getReg();
     if (OpReg == Reg)
       containsReg = true;
     // Anything other than a low register isn't legal here.
     if (!isARMLowRegister(OpReg) && (!HiReg || OpReg != HiReg))
       return true;
   }
   return false;
 }
 
 // Check if the specified regisgter is in the register list of the inst,
 // starting at the indicated operand number.
-static bool listContainsReg(MCInst &Inst, unsigned OpNo, unsigned Reg) {
-  for (unsigned i = OpNo; i < Inst.getNumOperands(); ++i) {
+static bool listContainsReg(const MCInst &Inst, unsigned OpNo, unsigned Reg) {
+  for (unsigned i = OpNo, e = Inst.getNumOperands(); i < e; ++i) {
     unsigned OpReg = Inst.getOperand(i).getReg();
     if (OpReg == Reg)
       return true;
   }
   return false;
 }
 
 // Return true if instruction has the interesting property of being
 // allowed in IT blocks, but not being predicable.
 static bool instIsBreakpoint(const MCInst &Inst) {
     return Inst.getOpcode() == ARM::tBKPT ||
            Inst.getOpcode() == ARM::BKPT ||
            Inst.getOpcode() == ARM::tHLT ||
            Inst.getOpcode() == ARM::HLT;
 
 }
 
-bool ARMAsmParser::validatetLDMRegList(MCInst Inst,
+bool ARMAsmParser::validatetLDMRegList(const MCInst &Inst,
                                        const OperandVector &Operands,
                                        unsigned ListNo, bool IsARPop) {
   const ARMOperand &Op = static_cast<const ARMOperand &>(*Operands[ListNo]);
   bool HasWritebackToken = Op.isToken() && Op.getToken() == "!";
 
   bool ListContainsSP = listContainsReg(Inst, ListNo, ARM::SP);
   bool ListContainsLR = listContainsReg(Inst, ListNo, ARM::LR);
   bool ListContainsPC = listContainsReg(Inst, ListNo, ARM::PC);
 
   if (!IsARPop && ListContainsSP)
     return Error(Operands[ListNo + HasWritebackToken]->getStartLoc(),
                  "SP may not be in the register list");
   else if (ListContainsPC && ListContainsLR)
     return Error(Operands[ListNo + HasWritebackToken]->getStartLoc(),
                  "PC and LR may not be in the register list simultaneously");
   else if (inITBlock() && !lastInITBlock() && ListContainsPC)
     return Error(Operands[ListNo + HasWritebackToken]->getStartLoc(),
                  "instruction must be outside of IT block or the last "
                  "instruction in an IT block");
   return false;
 }
 
-bool ARMAsmParser::validatetSTMRegList(MCInst Inst,
+bool ARMAsmParser::validatetSTMRegList(const MCInst &Inst,
                                        const OperandVector &Operands,
                                        unsigned ListNo) {
   const ARMOperand &Op = static_cast<const ARMOperand &>(*Operands[ListNo]);
   bool HasWritebackToken = Op.isToken() && Op.getToken() == "!";
 
   bool ListContainsSP = listContainsReg(Inst, ListNo, ARM::SP);
   bool ListContainsPC = listContainsReg(Inst, ListNo, ARM::PC);