[llvm] r278478 - ADT: Remove all ilist_iterator => pointer casts, NFC
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 11 22:05:36 PDT 2016
Author: dexonsmith
Date: Fri Aug 12 00:05:36 2016
New Revision: 278478
URL: http://llvm.org/viewvc/llvm-project?rev=278478&view=rev
Log:
ADT: Remove all ilist_iterator => pointer casts, NFC
Remove all ilist_iterator to pointer casts. There were two reasons for
casts:
- Checking for an uninitialized (i.e., null) iterator. I added
MachineInstrBundleIterator::isValid() to check for that case.
- Comparing an iterator against the underlying pointer value while
avoiding converting the pointer value to an iterator. This is
occasionally necessary in MachineInstrBundleIterator, since there is
an assertion in the constructors that the underlying MachineInstr is
not bundled (but we don't care about that if we're just checking for
pointer equality).
To support the latter case, I rewrote the == and != operators for
ilist_iterator and MachineInstrBundleIterator.
- The implicit constructors now use enable_if to exclude
const-iterator => non-const-iterator conversions from overload
resolution (previously it was a compiler error on instantiation, now
it's SFINAE).
- The == and != operators are now global (friends), and are not
templated.
- MachineInstrBundleIterator has overloads to compare against both
const_pointer and const_reference. This avoids the implicit
conversions to MachineInstrBundleIterator that assert, instead just
checking the address (and I added unit tests to confirm this).
Notably, the only remaining uses of ilist_iterator::getNodePtrUnchecked
are in ilist.h, and no code outside of ilist*.h directly relies on this
UB end-iterator-to-pointer conversion anymore. It's still needed for
ilist_*sentinel_traits, but I'll clean that up soon.
Added:
llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp
Modified:
llvm/trunk/include/llvm/ADT/ilist.h
llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h
llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
llvm/trunk/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp
llvm/trunk/lib/Target/AMDGPU/R600InstrInfo.cpp
llvm/trunk/lib/Target/Hexagon/HexagonCopyToCombine.cpp
llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp
llvm/trunk/unittests/CodeGen/CMakeLists.txt
Modified: llvm/trunk/include/llvm/ADT/ilist.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ilist.h?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/ilist.h (original)
+++ llvm/trunk/include/llvm/ADT/ilist.h Fri Aug 12 00:05:36 2016
@@ -43,6 +43,7 @@
#include <cassert>
#include <cstddef>
#include <iterator>
+#include <type_traits>
namespace llvm {
@@ -210,6 +211,9 @@ public:
typedef typename super::pointer pointer;
typedef typename super::reference reference;
+ typedef typename std::add_const<value_type>::type *const_pointer;
+ typedef typename std::add_const<value_type>::type &const_reference;
+
typedef typename ilist_detail::ConstCorrectNodeType<NodeTy>::type node_type;
typedef node_type *node_pointer;
typedef node_type &node_reference;
@@ -229,7 +233,10 @@ public:
// This is templated so that we can allow constructing a const iterator from
// a nonconst iterator...
template <class node_ty>
- ilist_iterator(const ilist_iterator<node_ty> &RHS)
+ ilist_iterator(
+ const ilist_iterator<node_ty> &RHS,
+ typename std::enable_if<std::is_convertible<node_ty *, NodeTy *>::value,
+ void *>::type = nullptr)
: NodePtr(RHS.getNodePtrUnchecked()) {}
// This is templated so that we can allow assigning to a const iterator from
@@ -243,16 +250,15 @@ public:
void reset(pointer NP) { NodePtr = NP; }
// Accessors...
- explicit operator pointer() const { return NodePtr; }
reference operator*() const { return *NodePtr; }
pointer operator->() const { return &operator*(); }
// Comparison operators
- template <class Y> bool operator==(const ilist_iterator<Y> &RHS) const {
- return NodePtr == RHS.getNodePtrUnchecked();
+ friend bool operator==(const ilist_iterator &LHS, const ilist_iterator &RHS) {
+ return LHS.NodePtr == RHS.NodePtr;
}
- template <class Y> bool operator!=(const ilist_iterator<Y> &RHS) const {
- return NodePtr != RHS.getNodePtrUnchecked();
+ friend bool operator!=(const ilist_iterator &LHS, const ilist_iterator &RHS) {
+ return LHS.NodePtr != RHS.NodePtr;
}
// Increment and decrement operators...
Modified: llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h (original)
+++ llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h Fri Aug 12 00:05:36 2016
@@ -24,18 +24,27 @@ namespace llvm {
template <typename Ty>
class MachineInstrBundleIterator
: public std::iterator<std::bidirectional_iterator_tag, Ty, ptrdiff_t> {
+ typedef std::iterator<std::bidirectional_iterator_tag, Ty, ptrdiff_t> super;
typedef ilist_iterator<Ty> instr_iterator;
instr_iterator MII;
public:
+ typedef typename super::value_type value_type;
+ typedef typename super::difference_type difference_type;
+ typedef typename super::pointer pointer;
+ typedef typename super::reference reference;
+
+ typedef typename instr_iterator::const_pointer const_pointer;
+ typedef typename instr_iterator::const_reference const_reference;
+
MachineInstrBundleIterator(instr_iterator MI) : MII(MI) {}
- MachineInstrBundleIterator(Ty &MI) : MII(MI) {
+ MachineInstrBundleIterator(reference MI) : MII(MI) {
assert(!MI.isBundledWithPred() && "It's not legal to initialize "
"MachineInstrBundleIterator with a "
"bundled MI");
}
- MachineInstrBundleIterator(Ty *MI) : MII(MI) {
+ MachineInstrBundleIterator(pointer MI) : MII(MI) {
// FIXME: This conversion should be explicit.
assert((!MI || !MI->isBundledWithPred()) && "It's not legal to initialize "
"MachineInstrBundleIterator "
@@ -43,21 +52,57 @@ public:
}
// Template allows conversion from const to nonconst.
template <class OtherTy>
- MachineInstrBundleIterator(const MachineInstrBundleIterator<OtherTy> &I)
+ MachineInstrBundleIterator(
+ const MachineInstrBundleIterator<OtherTy> &I,
+ typename std::enable_if<std::is_convertible<OtherTy *, Ty *>::value,
+ void *>::type = nullptr)
: MII(I.getInstrIterator()) {}
MachineInstrBundleIterator() : MII(nullptr) {}
- Ty &operator*() const { return *MII; }
- Ty *operator->() const { return &operator*(); }
+ reference operator*() const { return *MII; }
+ pointer operator->() const { return &operator*(); }
- // FIXME: This should be implemented as "return &operator*()" (or removed).
- explicit operator Ty *() const { return MII.getNodePtrUnchecked(); }
+ /// Check for null.
+ bool isValid() const { return MII.getNodePtr(); }
- bool operator==(const MachineInstrBundleIterator &X) const {
- return MII == X.MII;
+ friend bool operator==(const MachineInstrBundleIterator &L,
+ const MachineInstrBundleIterator &R) {
+ return L.MII == R.MII;
+ }
+ friend bool operator==(const MachineInstrBundleIterator &L, const_pointer R) {
+ // Avoid assertion about validity of R.
+ return L.MII == instr_iterator(const_cast<pointer>(R));
+ }
+ friend bool operator==(const_pointer L, const MachineInstrBundleIterator &R) {
+ // Avoid assertion about validity of L.
+ return instr_iterator(const_cast<pointer>(L)) == R.MII;
+ }
+ friend bool operator==(const MachineInstrBundleIterator &L,
+ const_reference R) {
+ return L == &R; // Avoid assertion about validity of R.
+ }
+ friend bool operator==(const_reference L,
+ const MachineInstrBundleIterator &R) {
+ return &L == R; // Avoid assertion about validity of L.
+ }
+
+ friend bool operator!=(const MachineInstrBundleIterator &L,
+ const MachineInstrBundleIterator &R) {
+ return !(L == R);
+ }
+ friend bool operator!=(const MachineInstrBundleIterator &L, const_pointer R) {
+ return !(L == R);
+ }
+ friend bool operator!=(const_pointer L, const MachineInstrBundleIterator &R) {
+ return !(L == R);
+ }
+ friend bool operator!=(const MachineInstrBundleIterator &L,
+ const_reference R) {
+ return !(L == R);
}
- bool operator!=(const MachineInstrBundleIterator &X) const {
- return !operator==(X);
+ friend bool operator!=(const_reference L,
+ const MachineInstrBundleIterator &R) {
+ return !(L == R);
}
// Increment and decrement operators...
Modified: llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp Fri Aug 12 00:05:36 2016
@@ -353,8 +353,8 @@ void FastISel::recomputeInsertPt() {
void FastISel::removeDeadCode(MachineBasicBlock::iterator I,
MachineBasicBlock::iterator E) {
- assert(static_cast<MachineInstr *>(I) && static_cast<MachineInstr *>(E) &&
- std::distance(I, E) > 0 && "Invalid iterator!");
+ assert(I.isValid() && E.isValid() && std::distance(I, E) > 0 &&
+ "Invalid iterator!");
while (I != E) {
MachineInstr *Dead = &*I;
++I;
Modified: llvm/trunk/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp (original)
+++ llvm/trunk/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp Fri Aug 12 00:05:36 2016
@@ -423,25 +423,21 @@ bool AMDGPUCFGStructurizer::needMigrateB
void AMDGPUCFGStructurizer::reversePredicateSetter(
MachineBasicBlock::iterator I) {
- assert(static_cast<MachineInstr *>(I) && "Expected valid iterator");
+ assert(I.isValid() && "Expected valid iterator");
for (;; --I) {
if (I->getOpcode() == AMDGPU::PRED_X) {
- switch (static_cast<MachineInstr *>(I)->getOperand(2).getImm()) {
+ switch (I->getOperand(2).getImm()) {
case OPCODE_IS_ZERO_INT:
- static_cast<MachineInstr *>(I)->getOperand(2)
- .setImm(OPCODE_IS_NOT_ZERO_INT);
+ I->getOperand(2).setImm(OPCODE_IS_NOT_ZERO_INT);
return;
case OPCODE_IS_NOT_ZERO_INT:
- static_cast<MachineInstr *>(I)->getOperand(2)
- .setImm(OPCODE_IS_ZERO_INT);
+ I->getOperand(2).setImm(OPCODE_IS_ZERO_INT);
return;
case OPCODE_IS_ZERO:
- static_cast<MachineInstr *>(I)->getOperand(2)
- .setImm(OPCODE_IS_NOT_ZERO);
+ I->getOperand(2).setImm(OPCODE_IS_NOT_ZERO);
return;
case OPCODE_IS_NOT_ZERO:
- static_cast<MachineInstr *>(I)->getOperand(2)
- .setImm(OPCODE_IS_ZERO);
+ I->getOperand(2).setImm(OPCODE_IS_ZERO);
return;
default:
llvm_unreachable("PRED_X Opcode invalid!");
Modified: llvm/trunk/lib/Target/AMDGPU/R600InstrInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/R600InstrInfo.cpp?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AMDGPU/R600InstrInfo.cpp (original)
+++ llvm/trunk/lib/Target/AMDGPU/R600InstrInfo.cpp Fri Aug 12 00:05:36 2016
@@ -665,7 +665,7 @@ bool R600InstrInfo::analyzeBranch(Machin
// handled
if (isBranch(I->getOpcode()))
return true;
- if (!isJump(static_cast<MachineInstr *>(I)->getOpcode())) {
+ if (!isJump(I->getOpcode())) {
return false;
}
@@ -680,8 +680,7 @@ bool R600InstrInfo::analyzeBranch(Machin
// If there is only one terminator instruction, process it.
unsigned LastOpc = LastInst.getOpcode();
- if (I == MBB.begin() ||
- !isJump(static_cast<MachineInstr *>(--I)->getOpcode())) {
+ if (I == MBB.begin() || !isJump((--I)->getOpcode())) {
if (LastOpc == AMDGPU::JUMP) {
TBB = LastInst.getOperand(0).getMBB();
return false;
Modified: llvm/trunk/lib/Target/Hexagon/HexagonCopyToCombine.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Hexagon/HexagonCopyToCombine.cpp?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/lib/Target/Hexagon/HexagonCopyToCombine.cpp (original)
+++ llvm/trunk/lib/Target/Hexagon/HexagonCopyToCombine.cpp Fri Aug 12 00:05:36 2016
@@ -606,7 +606,7 @@ void HexagonCopyToCombine::combine(Machi
for (auto NewMI : DbgMItoMove) {
// If iterator MI is pointing to DEBUG_VAL, make sure
// MI now points to next relevant instruction.
- if (NewMI == (MachineInstr*)MI)
+ if (NewMI == MI)
++MI;
BB->splice(InsertPt, BB, NewMI);
}
Modified: llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp (original)
+++ llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp Fri Aug 12 00:05:36 2016
@@ -1301,8 +1301,7 @@ void MipsConstantIslands::createNewWater
Offset < BaseInsertOffset;
Offset += TII->getInstSizeInBytes(*MI), MI = std::next(MI)) {
assert(MI != UserMBB->end() && "Fell off end of block");
- if (CPUIndex < NumCPUsers &&
- CPUsers[CPUIndex].MI == static_cast<MachineInstr *>(MI)) {
+ if (CPUIndex < NumCPUsers && CPUsers[CPUIndex].MI == MI) {
CPUser &U = CPUsers[CPUIndex];
if (!isOffsetInRange(Offset, EndInsertOffset, U)) {
// Shift intertion point by one unit of alignment so it is within reach.
Modified: llvm/trunk/unittests/CodeGen/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/CodeGen/CMakeLists.txt?rev=278478&r1=278477&r2=278478&view=diff
==============================================================================
--- llvm/trunk/unittests/CodeGen/CMakeLists.txt (original)
+++ llvm/trunk/unittests/CodeGen/CMakeLists.txt Fri Aug 12 00:05:36 2016
@@ -8,6 +8,7 @@ set(LLVM_LINK_COMPONENTS
set(CodeGenSources
DIEHashTest.cpp
LowLevelTypeTest.cpp
+ MachineInstrBundleIteratorTest.cpp
)
add_llvm_unittest(CodeGenTests
Added: llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp?rev=278478&view=auto
==============================================================================
--- llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp (added)
+++ llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp Fri Aug 12 00:05:36 2016
@@ -0,0 +1,94 @@
+//===- MachineInstrBundleIteratorTest.cpp ---------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/ilist_node.h"
+#include "llvm/CodeGen/MachineInstrBundleIterator.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+struct MyBundledInstr : public ilist_node<MyBundledInstr> {
+ bool isBundledWithPred() const { return true; }
+ bool isBundledWithSucc() const { return true; }
+};
+typedef MachineInstrBundleIterator<MyBundledInstr> bundled_iterator;
+typedef MachineInstrBundleIterator<const MyBundledInstr> const_bundled_iterator;
+
+#ifdef GTEST_HAS_DEATH_TEST
+#ifndef NDEBUG
+TEST(MachineInstrBundleIteratorTest, CheckForBundles) {
+ MyBundledInstr MBI;
+
+ // Confirm that MBI is always considered bundled.
+ EXPECT_TRUE(MBI.isBundledWithPred());
+ EXPECT_TRUE(MBI.isBundledWithSucc());
+
+ // Confirm that iterators check in their constructor for bundled iterators.
+ EXPECT_DEATH((void)static_cast<bundled_iterator>(MBI),
+ "not legal to initialize");
+ EXPECT_DEATH((void)static_cast<bundled_iterator>(&MBI),
+ "not legal to initialize");
+ EXPECT_DEATH((void)static_cast<const_bundled_iterator>(MBI),
+ "not legal to initialize");
+ EXPECT_DEATH((void)static_cast<const_bundled_iterator>(&MBI),
+ "not legal to initialize");
+}
+#endif
+#endif
+
+TEST(MachineInstrBundleIteratorTest, CompareToBundledMI) {
+ MyBundledInstr MBI;
+ const MyBundledInstr &CMBI = MBI;
+ bundled_iterator I;
+ const_bundled_iterator CI;
+
+ // Confirm that MBI is always considered bundled.
+ EXPECT_TRUE(MBI.isBundledWithPred());
+ EXPECT_TRUE(MBI.isBundledWithSucc());
+
+ // These invocations will crash when !NDEBUG if a conversion is taking place.
+ // These checks confirm that comparison operators don't use any conversion
+ // operators.
+ ASSERT_FALSE(MBI == I);
+ ASSERT_FALSE(&MBI == I);
+ ASSERT_FALSE(CMBI == I);
+ ASSERT_FALSE(&CMBI == I);
+ ASSERT_FALSE(I == MBI);
+ ASSERT_FALSE(I == &MBI);
+ ASSERT_FALSE(I == CMBI);
+ ASSERT_FALSE(I == &CMBI);
+ ASSERT_FALSE(MBI == CI);
+ ASSERT_FALSE(&MBI == CI);
+ ASSERT_FALSE(CMBI == CI);
+ ASSERT_FALSE(&CMBI == CI);
+ ASSERT_FALSE(CI == MBI);
+ ASSERT_FALSE(CI == &MBI);
+ ASSERT_FALSE(CI == CMBI);
+ ASSERT_FALSE(CI == &CMBI);
+ ASSERT_TRUE(MBI != I);
+ ASSERT_TRUE(&MBI != I);
+ ASSERT_TRUE(CMBI != I);
+ ASSERT_TRUE(&CMBI != I);
+ ASSERT_TRUE(I != MBI);
+ ASSERT_TRUE(I != &MBI);
+ ASSERT_TRUE(I != CMBI);
+ ASSERT_TRUE(I != &CMBI);
+ ASSERT_TRUE(MBI != CI);
+ ASSERT_TRUE(&MBI != CI);
+ ASSERT_TRUE(CMBI != CI);
+ ASSERT_TRUE(&CMBI != CI);
+ ASSERT_TRUE(CI != MBI);
+ ASSERT_TRUE(CI != &MBI);
+ ASSERT_TRUE(CI != CMBI);
+ ASSERT_TRUE(CI != &CMBI);
+}
+
+} // end namespace
More information about the llvm-commits
mailing list