BumpPtrAllocator: don't accept 0 for the alignment parameter

[hans]

Summary

It seems unnecessary to have to use an extra branch to check for this special case.

I couldn't measure any performance difference. Perhaps Clang inlines and removes the check most of the time, but I still think it's worth fixing.

[majnemer]

LGTM.

[hans]

This was committed in r216036.

include/llvm/Support/Allocator.h

void Reset() {
     DeallocateSlabs(std::next(Slabs.begin()), Slabs.end());
     Slabs.erase(std::next(Slabs.begin()), Slabs.end());
     DeallocateCustomSizedSlabs();
     CustomSizedSlabs.clear();
   }
 
   /// \brief Allocate space at the specified alignment.
   void *Allocate(size_t Size, size_t Alignment) {
+    assert(Alignment > 0 && "0-byte alignnment is not allowed. Use 1 instead.");
+
     // Keep track of how many bytes we've allocated.
     BytesAllocated += Size;
 
-    // 0-byte alignment means 1-byte alignment.
-    if (Alignment == 0)
-      Alignment = 1;
-
     // Allocate the aligned space, going forwards from CurPtr.
     char *Ptr = alignPtr(CurPtr, Alignment);
 
     // Check if we can hold it.
     if (Ptr + Size <= End) {
       CurPtr = Ptr + Size;
       // Update the allocation point of this memory block in MemorySanitizer.
       // Without this, MemorySanitizer messages for values originated from here

unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp

TEST(JITMemoryManagerTest, TestCodeAllocation) {
   EXPECT_TRUE(MemMgr->CheckInvariants(Error)) << Error;
 }
 
 // Allocate five global ints of varying widths and alignment, and check their
 // alignment and overlap.
 TEST(JITMemoryManagerTest, TestSmallGlobalInts) {
   std::unique_ptr<JITMemoryManager> MemMgr(
       JITMemoryManager::CreateDefaultMemManager());
-  uint8_t  *a = (uint8_t *)MemMgr->allocateGlobal(8,  0);
+  uint8_t  *a = (uint8_t *)MemMgr->allocateGlobal(8,  1);
   uint16_t *b = (uint16_t*)MemMgr->allocateGlobal(16, 2);
   uint32_t *c = (uint32_t*)MemMgr->allocateGlobal(32, 4);
   uint64_t *d = (uint64_t*)MemMgr->allocateGlobal(64, 8);
 
   // Check the alignment.
   EXPECT_EQ(0U, ((uintptr_t)b) & 0x1);
   EXPECT_EQ(0U, ((uintptr_t)c) & 0x3);
   EXPECT_EQ(0U, ((uintptr_t)d) & 0x7);

unittests/Support/AllocatorTest.cpp

 #include <cstdlib>
 
 using namespace llvm;
 
 namespace {
 
 TEST(AllocatorTest, Basics) {
   BumpPtrAllocator Alloc;
-  int *a = (int*)Alloc.Allocate(sizeof(int), 0);
-  int *b = (int*)Alloc.Allocate(sizeof(int) * 10, 0);
-  int *c = (int*)Alloc.Allocate(sizeof(int), 0);
+  int *a = (int*)Alloc.Allocate(sizeof(int), 1);
+  int *b = (int*)Alloc.Allocate(sizeof(int) * 10, 1);
+  int *c = (int*)Alloc.Allocate(sizeof(int), 1);
   *a = 1;
   b[0] = 2;
   b[9] = 2;
   *c = 3;
   EXPECT_EQ(1, *a);
   EXPECT_EQ(2, b[0]);
   EXPECT_EQ(2, b[9]);
   EXPECT_EQ(3, *c);
TEST(AllocatorTest, Basics) {
   Alloc = std::move(Alloc2);
   EXPECT_EQ(0U, Alloc2.GetNumSlabs());
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
 }
 
 // Allocate enough bytes to create three slabs.
 TEST(AllocatorTest, ThreeSlabs) {
   BumpPtrAllocator Alloc;
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(2U, Alloc.GetNumSlabs());
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(3U, Alloc.GetNumSlabs());
 }
 
 // Allocate enough bytes to create two slabs, reset the allocator, and do it
 // again.
 TEST(AllocatorTest, TestReset) {
   BumpPtrAllocator Alloc;
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(2U, Alloc.GetNumSlabs());
   Alloc.Reset();
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
-  Alloc.Allocate(3000, 0);
+  Alloc.Allocate(3000, 1);
   EXPECT_EQ(2U, Alloc.GetNumSlabs());
 }
 
 // Test some allocations at varying alignments.
 TEST(AllocatorTest, TestAlignment) {
   BumpPtrAllocator Alloc;
   uintptr_t a;
   a = (uintptr_t)Alloc.Allocate(1, 2);

 }
 
 // Test allocating just over the slab size.  This tests a bug where before the
 // allocator incorrectly calculated the buffer end pointer.
 TEST(AllocatorTest, TestOverflow) {
   BumpPtrAllocator Alloc;
 
   // Fill the slab right up until the end pointer.
-  Alloc.Allocate(4096, 0);
+  Alloc.Allocate(4096, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
 
   // If we don't allocate a new slab, then we will have overflowed.
-  Alloc.Allocate(1, 0);
+  Alloc.Allocate(1, 1);
   EXPECT_EQ(2U, Alloc.GetNumSlabs());
 }
 
 // Test allocating with a size larger than the initial slab size.
 TEST(AllocatorTest, TestSmallSlabSize) {
   BumpPtrAllocator Alloc;
 
-  Alloc.Allocate(8000, 0);
+  Alloc.Allocate(8000, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
 }
 
 // Mock slab allocator that returns slabs aligned on 4096 bytes.  There is no
 // easy portable way to do this, so this is kind of a hack.
 class MockSlabAllocator {
   static size_t LastSlabSize;
 

 
 // Allocate a large-ish block with a really large alignment so that the
 // allocator will think that it has space, but after it does the alignment it
 // will not.
 TEST(AllocatorTest, TestBigAlignment) {
   BumpPtrAllocatorImpl<MockSlabAllocator> Alloc;
 
   // First allocate a tiny bit to ensure we have to re-align things.
-  (void)Alloc.Allocate(1, 0);
+  (void)Alloc.Allocate(1, 1);
 
   // Now the big chunk with a big alignment.
   (void)Alloc.Allocate(3000, 2048);
 
   // We test that the last slab size is not the default 4096 byte slab, but
   // rather a custom sized slab that is larger.
   EXPECT_GT(MockSlabAllocator::GetLastSlabSize(), 4096u);
 }
 
 }  // anonymous namespace