Notice that it has an 8-byte alignment. However, the Bar variable has
1-byte alignment, because it's being placed into the "redzone". The
generic code for the function is:
I don't get it Bill. llvm should be making memcpy with the right alignment regardless of how Bar is formed. Changing the alignment of bar papers over this problem, but we will still get improper alignment for other cases. Also, DECL_USER_ALIGN isn't appropriate to tweak here... the user isn't playing around with __attribute__((aligned))
Bar isn't the one that's getting realigned with the above code, it's the constructor. The patch only sets the DECL_USER_ALIGN field for the constructor (not Bar) when the constructor's created. My thoughts were that if the constructor was initially given the alignment of the variable (Bar, in this case), then it shouldn't be changed in the assemble_variable() function, unless it's also changing the alignment of Bar (which it's not). I'm actually not sure why it doesn't change Bar's alignment when it's changing Bar's constructor's alignment. That seems like a GCC bug, IMHO, though they probably don't get hit with it or "fix" it later on or something.
My guess is that you want something akin to TreeToLLVM::EmitBuiltinMemCopy where it gets the min of the alignments?
I don't get it Bill. llvm should be making memcpy with the right
alignment regardless of how Bar is formed. Changing the alignment of
bar papers over this problem, but we will still get improper alignment
for other cases. Also, DECL_USER_ALIGN isn't appropriate to tweak
here... the user isn't playing around with __attribute__((aligned))
the fundamental problem is that the DestLoc argument to Emit doesn't
come with alignment (or volatility for that matter). When emitting
an aggregate into DestLoc only the alignment of the source is used,
not that of DestLoc. But that's wrong if DestLoc is less aligned
than the source (either because the source is highly aligned, eg
because the user marked it as being highly aligned, or because the
destination is little aligned as in the testcase). So why not pass
the alignment and volatility in too? Probably Chris doesn't want
to add extra arguments to Emit, so instead we could define a
"DestLoc descriptor" struct:
struct DLD {
Value *Ptr;
unsigned Alignment;
bool Volatile;
}
declare one on the stack and pass its address, something like this:
DLD D = { Ptr, Align, isVolatile };
Emit(exp, &D);
OK then, it looks like other people have made the points I was going to, but briefly:
Using DECL_USER_ALIGN is wrong; that is for user-specified alignments, and if
you use it for something else, you'll break that.
The alignment on memcpy needs to take both the source & destination into account.
You seem to have gotten this far. Unfortunately that bit of llvm-convert is a maze of
recursive little passages, but your last patch seems to be thinking along the right lines.
Would it be terrible to drop the alignment on memcpy altogether? The source and
destination have alignments and llvm should be able to figure out what the safe
alignment is for memcpy. This is a target-dependent decision anyway; misaligned
loads are a good idea on some targets, not on others.
> I don't get it Bill. llvm should be making memcpy with the right
> alignment regardless of how Bar is formed. Changing the alignment of
> bar papers over this problem, but we will still get improper alignment
> for other cases. Also, DECL_USER_ALIGN isn't appropriate to tweak
> here... the user isn't playing around with __attribute__((aligned))
the fundamental problem is that the DestLoc argument to Emit doesn't
come with alignment (or volatility for that matter). When emitting
an aggregate into DestLoc only the alignment of the source is used,
not that of DestLoc. But that's wrong if DestLoc is less aligned
than the source (either because the source is highly aligned, eg
because the user marked it as being highly aligned, or because the
destination is little aligned as in the testcase). So why not pass
the alignment and volatility in too? Probably Chris doesn't want
to add extra arguments to Emit, so instead we could define a
"DestLoc descriptor" struct:
struct DLD {
Value *Ptr;
unsigned Alignment;
bool Volatile;
}
declare one on the stack and pass its address, something like this:
DLD D = { Ptr, Align, isVolatile };
Emit(exp, &D);
I just did this in a sudden fit of activity. It was quite easy and
looks like it catches a ton of volatility and alignment bugs. Maybe
it will even work and fix the original problem I will let you
know later.
How about this one It passes alignment and volatility around
with DestLoc. It's not finished because I noticed some bugs in
how CopyAggregate and ZeroAggregate handle alignment (problem points
marked with "QQ"). Also, I noticed potential problems with how we
handle call arguments and return results (what if they are strangely
aligned/volatile?), marked with FIXME.
While there, I fixed the following:
- Taught EmitAggregateZero not to do element by element zeroing
if the LLVM type doesn't cover the gcc type. Also, not to do
it if there are too many aggregate elements.
- Added a bunch of missing alignment/volatile stuff on various
loads and stores.
- Fixed up a lot of bogus handling of volatility in the complex
number stuff.
the fundamental problem is that the DestLoc argument to Emit doesn't
come with alignment (or volatility for that matter). When emitting
an aggregate into DestLoc only the alignment of the source is used,
not that of DestLoc. But that's wrong if DestLoc is less aligned
than the source (either because the source is highly aligned, eg
because the user marked it as being highly aligned, or because the
destination is little aligned as in the testcase).
Exactly!
So why not pass the alignment and volatility in too?
I'm for that.
Probably Chris doesn't want
to add extra arguments to Emit, so instead we could define a
"DestLoc descriptor" struct:
struct DLD {
Value *Ptr;
unsigned Alignment;
bool Volatile;
}
declare one on the stack and pass its address, something like this:
DLD D = { Ptr, Align, isVolatile };
Emit(exp, &D);
Emit already takes a parameter that not all of the method calls in
there use. So it should be possible to add another parameter. I
submitted another patch for review that does that -- at least for the
alignment.
OK then, it looks like other people have made the points I was going
to, but briefly:
Using DECL_USER_ALIGN is wrong; that is for user-specified
alignments, and if
you use it for something else, you'll break that.
Okay.
The alignment on memcpy needs to take both the source & destination
into account.
I agree. But I was thinking that there was a more fundamental problem,
i.e. that GCC was changing the alignment of the constructor without
taking the destination's alignment into account -- that is, it's not
changing the destination's alignment and/or checking it beforehand.
You seem to have gotten this far. Unfortunately that bit of llvm-
convert is a maze of
recursive little passages,
Tell me about it!
but your last patch seems to be thinking along the right lines.
No, the compiler has complete control of the alignment of compiler-generated
"constructors" (in gcc's terminology; they have nothing to do with C++
constructors) and normally aligns them restrictively for efficiency. There is no
reason the alignment of the destination should be the same.
I will let you
