Assertion failed: !(isAtomic() && getAlignment() == 0) && "Alignment required for atomic load"

[llvmbot]

Bugzilla Link	45010
Resolution	FIXED
Resolved on	Feb 28, 2020 02:35
Version	10.0
OS	Windows NT
Blocks	#43900
Reporter	LLVM Bugzilla Contributor
CC	@chandlerc,@zmodem,@jfbastien,@zygoloid

Extended Description

When compiling Clang with assertions enabled, I'm seeing the assert I've mentioned here: https://reviews.llvm.org/D71547#1824272

Repro:

// a.cpp
// Build with: clang-cl a.cpp /c /O1
#include
struct SP {
void WeakAddRef();
void WeakRelease();
};
template struct PtrBase {
T *volatile P;
~PtrBase() {
if (R)
R->WeakRelease();
}
PtrBase(const PtrBase &O) : R(O.R) {
if (R)
R->WeakAddRef();
}
SP *R;
bool IsValid() const { return P != nullptr; }
};
struct alignas(16) VectorSIMD4f {
float V;
};
unsigned __int64 countAF() {
class AF {
VectorSIMD4f B;
};
struct PD {
PtrBase P;
};
std::vector AFs;
unsigned __int64 C = 0;
for (auto A : AFs) {
if (A.P.IsValid())
++C;
}
return C;
}

[zmodem]

Repro without the std::vector dependency:

$ cat /tmp/a.cc
template struct Vector {
T t;
unsigned n;
Vector() : t(nullptr), n(0) {}
T begin() { return t; }
T* end() { return t + n; }
};

struct SP {
void WeakAddRef();
void WeakRelease();
};
template struct PtrBase {
T *volatile P;
~PtrBase() {
if (R)
R->WeakRelease();
}
PtrBase(const PtrBase &O) : R(O.R) {
if (R)
R->WeakAddRef();
}
SP *R;
bool IsValid() const { return P != nullptr; }
};
struct alignas(16) VectorSIMD4f {
float V;
};
unsigned __int64 countAF() {
class AF {
VectorSIMD4f B;
};
struct PD {
PtrBase P;
};
Vector AFs;
unsigned __int64 C = 0;
for (auto A : AFs) {
if (A.P.IsValid())
++C;
}
return C;
}

$ bin/clang -cc1 -triple i686-pc-win32 -emit-obj -fms-volatile -Os -fms-extensions /tmp/a.cc
clang: /work/llvm.monorepo/llvm/lib/IR/Instructions.cpp:1293: void llvm::LoadInst::AssertOK(): Assertion `!(isAtomic() && getAlignment() == 0) && "Alignment required for atomic load"' failed.

[zmodem]

Guillaume, can you take a look?

[gchatelet]

Yes, looking into it.

[gchatelet]

I've reduced it to:

template <typename T> struct Vector {
  T *t;
  unsigned n;
  Vector() : t(nullptr), n(0) {}
  T *begin() { return t; }
  T *end() { return t + n; }
};

struct SP {
  void WeakAddRef();
  void WeakRelease();
};

template <typename T> struct PtrBase {
  T *volatile P;
  SP *R;
  ~PtrBase() {
    if (R)
      R->WeakRelease();
  }
  PtrBase(const PtrBase &O) : R(O.R) {
    if (R)
      R->WeakAddRef();
  }
};

using PD = PtrBase<float>;

bool countAF() {
  Vector<PD> AFs;
  for (auto A : AFs)
    if (A.P)
      return true;
  return false;
}

I'm still digging into it.

[gchatelet]

Reduced even further

struct SP {
  void foo();
  void bar();
};

struct PtrBase {
  int *volatile ptr;
  SP *cp;
  ~PtrBase() {
    if (cp)
      cp->bar();
  }
  PtrBase(const PtrBase &O) : cp(O.cp) {
    if (cp)
      cp->foo();
  }
};

bool countAF(PtrBase *A) {
  auto copy = *A;
  return copy.ptr;
}

[gchatelet]

I narrowed it down to SROA pass that removes the alignment requirement on the alloca

clang -cc1 -triple i686-pc-win32 -fms-volatile -Os -emit-obj -mllvm -print-before-all -mllvm -print-after-all /tmp/reproducer.cpp

*** IR Dump Before SROA ***
; Function Attrs: nounwind optsize
define dso_local zeroext i1 @"?countAF@@YA_NPAUPtrBase@@@z"(%struct.PtrBase* nocapture readonly %A) local_unnamed_addr #​0 {
entry:
%copy = alloca %struct.PtrBase, align 4
%0 = bitcast %struct.PtrBase* %copy to i8*
call void @​llvm.lifetime.start.p0i8(i64 8, i8* nonnull %0) #​3
%cp.i = getelementptr inbounds %struct.PtrBase, %struct.PtrBase* %copy, i32 0, i32 1
%cp2.i = getelementptr inbounds %struct.PtrBase, %struct.PtrBase* %A, i32 0, i32 1
%1 = load %struct.SP*, %struct.SP** %cp2.i, align 4, !tbaa !​3
store %struct.SP* %1, %struct.SP** %cp.i, align 4, !tbaa !​3
%tobool.i = icmp eq %struct.SP* %1, null
br i1 %tobool.i, label %"??0PtrBase@@QAE@ABU0@@Z.exit", label %if.then.i

if.then.i: ; preds = %entry
call x86_thiscallcc void @"?foo@SP@@QAEXXZ"(%struct.SP* nonnull %1) #​4
br label %"??0PtrBase@@QAE@ABU0@@Z.exit"

"??0PtrBase@@QAE@ABU0@@Z.exit": ; preds = %entry, %if.then.i
%2 = bitcast %struct.PtrBase* %copy to i32*
%atomic-load = load atomic volatile i32, i32* %2 acquire, align 4, !tbaa !​8
%tobool = icmp ne i32 %atomic-load, 0
%cp.i1 = getelementptr inbounds %struct.PtrBase, %struct.PtrBase* %copy, i32 0, i32 1
%3 = load %struct.SP*, %struct.SP** %cp.i1, align 4, !tbaa !​3
%tobool.i2 = icmp eq %struct.SP* %3, null
br i1 %tobool.i2, label %"??1PtrBase@@QAE@XZ.exit", label %if.then.i3

if.then.i3: ; preds = %"??0PtrBase@@QAE@ABU0@@Z.exit"
call x86_thiscallcc void @"?bar@SP@@QAEXXZ"(%struct.SP* nonnull %3) #​4
br label %"??1PtrBase@@QAE@XZ.exit"

"??1PtrBase@@QAE@XZ.exit": ; preds = %"??0PtrBase@@QAE@ABU0@@Z.exit", %if.then.i3
call void @​llvm.lifetime.end.p0i8(i64 8, i8* nonnull %0) #​3
ret i1 %tobool
}

*** IR Dump After SROA ***
; Function Attrs: nounwind optsize
define dso_local zeroext i1 @"?countAF@@YA_NPAUPtrBase@@@z"(%struct.PtrBase* nocapture readonly %A) local_unnamed_addr #​0 {
entry:
%copy.sroa.0 = alloca i32
%copy.sroa.0.0..sroa_cast6 = bitcast i32* %copy.sroa.0 to i8*
call void @​llvm.lifetime.start.p0i8(i64 4, i8* %copy.sroa.0.0..sroa_cast6)
%cp2.i = getelementptr inbounds %struct.PtrBase, %struct.PtrBase* %A, i32 0, i32 1
%0 = load %struct.SP*, %struct.SP** %cp2.i, align 4, !tbaa !​3
%tobool.i = icmp eq %struct.SP* %0, null
br i1 %tobool.i, label %"??0PtrBase@@QAE@ABU0@@Z.exit", label %if.then.i

if.then.i: ; preds = %entry
call x86_thiscallcc void @"?foo@SP@@QAEXXZ"(%struct.SP* nonnull %0) #​3
br label %"??0PtrBase@@QAE@ABU0@@Z.exit"

"??0PtrBase@@QAE@ABU0@@Z.exit": ; preds = %entry, %if.then.i
%copy.sroa.0.0.copy.sroa.0.0.atomic-load = load atomic volatile i32, i32* %copy.sroa.0 acquire, !tbaa !​8
%tobool = icmp ne i32 %copy.sroa.0.0.copy.sroa.0.0.atomic-load, 0
%tobool.i2 = icmp eq %struct.SP* %0, null
br i1 %tobool.i2, label %"??1PtrBase@@QAE@XZ.exit", label %if.then.i3

if.then.i3: ; preds = %"??0PtrBase@@QAE@ABU0@@Z.exit"
call x86_thiscallcc void @"?bar@SP@@QAEXXZ"(%struct.SP* nonnull %0) #​3
br label %"??1PtrBase@@QAE@XZ.exit"

"??1PtrBase@@QAE@XZ.exit": ; preds = %"??0PtrBase@@QAE@ABU0@@Z.exit", %if.then.i3
%copy.sroa.0.0..sroa_cast7 = bitcast i32* %copy.sroa.0 to i8*
call void @​llvm.lifetime.end.p0i8(i64 4, i8* %copy.sroa.0.0..sroa_cast7)
ret i1 %tobool
}

[gchatelet]

I don't think this is a regression.
I went back to bdbb894 Thu Apr 20 20:54:19 2017 and I could still reproduce the failing assert.

Someone more knowledgeable than me on SROA should have a look.
It seems like the pass is dismissing the alignment on the A in the for loop.

[zmodem]

Someone more knowledgeable than me on SROA should have a look.
It seems like the pass is dismissing the alignment on the A in the for
loop.

Chandler is the SROA code owner. Do you know someone who could take a look at this?

[jfbastien]

Removing alignment requirements on atomic or volatile variables seems like a bug :)

[zmodem]

Smaller repro:

$ cat /tmp/a.ll
target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:32-n8:16:32-a:0:32-S32"
target triple = "i686-pc-windows-msvc"

%struct = type { i32*, i32 }

define dso_local i32* @​pr45010(%struct* %A) {
entry:
%B = alloca %struct, align 4

%A.i = getelementptr inbounds %struct, %struct* %A, i32 0, i32 0
%B.i = getelementptr inbounds %struct, %struct* %B, i32 0, i32 0
%0 = load i32*, i32** %A.i, align 4
store i32* %0, i32** %B.i, align 4

%1 = bitcast %struct* %B to i32**
%x = load atomic volatile i32*, i32** %1 acquire, align 4
ret i32* %x
}

$ bin/opt -sroa /tmp/a.ll -S
Atomic load must specify explicit alignment
%B.sroa.0.0.B.sroa.0.0.x = load atomic volatile i32*, i32** %B.sroa.0 acquire
in function pr45010
LLVM ERROR: Broken function found, compilation aborted!

[zmodem]

SROA has nice debug output that shows what's going on. From

$ bin/opt -sroa /tmp/a.ll -S -debug-only=sroa

Rewriting alloca partition [0,4) to: %B.sroa.0 = alloca i32*
rewriting [0,4) slice #​0
original: store i32* %0, i32** %B.i, align 4
to: store i32* %0, i32** %B.sroa.0
rewriting [0,4) slice #​1
original: %x = load atomic volatile i32*, i32** %1 acquire, align 4
to: %B.sroa.0.0.x = load atomic volatile i32*, i32** %B.sroa.0 acquire

The load is rewritten in SliceBuilder::visitLoadInst(), and it's using IRBuilder<>::CreateAlignedLoad to build the new one, feeding in the alignment of the new alloca, which in this case has been dropped.

The alignment gets dropped here: SROA::rewritePartition()

} else {
// If alignment is unspecified we fallback on the one required by the ABI
// for this type. We also make sure the alignment is compatible with
// P.beginOffset().
const Align Alignment = commonAlignment(
DL.getValueOrABITypeAlignment(MaybeAlign(AI.getAlignment()),
AI.getAllocatedType()),
P.beginOffset());
// If we will get at least this much alignment from the type alone, leave
// the alloca's alignment unconstrained.

I think this is where the problem sits. The code comes from Chandler's https://llvm.org/r164891

I wonder if it's okay to leave the alloca without the alignment, since the ABI should guarantee it anyway, and just make sure not to drop it on the load instruction...

[zmodem]

Patch: https://reviews.llvm.org/D75266

[zmodem]

Patch: https://reviews.llvm.org/D75266

Pushed to master as d48c981 and 10.x as 7cb6829.