Fix alignment of aggregates

[jobnoorman]

Summary

Codegen seems to assume aggregates are 8-byte alligned while placing them on the stack seems to only align them on 2 bytes. This causes code like the following to miscompile:

struct Foo {int a, b;};
int* i;

void g(void)
{
    struct Foo foo;
    i = &foo.b;
}

Here, i is assigned the address of foo.a.

This problem seems to be fixed by adding a0:16:16 to the data layout string.

Note that the patch doesn't include a test case since the above function is now compiled to

g:                                         ; @g
; BB#0:                                 ; %entry
        push.w  r4
        mov.w   r1, r4
        sub.w   #4, r1
        mov.w   r4, r12
        sub.w   #4, r12
        add.w   #2, r12
        mov.w   r12, &i
        add.w   #4, r1
        pop.w   r4
        ret

and I don't really want to encode this inefficient sub.w #4, r12, add.w #2, r12 in a test case. Any idea on how to create a decent test case for this?

[jobnoorman]

Note that I also have a patch for clang to update the data layout string but I'm not sure how to attach a second patch to this review.

[asl]

Can't you just use the IR after the optimization to check this?

[jobnoorman]

I'm afraid I'm not completely sure what you mean by this...

Note that the alignment in the IR was already correct (examples from the function shown above):

%foo = alloca %struct.Foo, align 2

However, the alignment seems to change after ISel:

.# *** IR Dump After Expand ISel Pseudo-instructions ***:
.# Machine code for function g: SSA
Frame Objects:
  fi#0: size=4, align=8, at location [SP+2] # <-- align is now 8

BB#0: derived from LLVM BB %entry
        %vreg0<def> = ADD16ri <fi#0>, 0, %SRW<imp-def,dead>; GR16:%vreg0
        %vreg1<def,tied1> = OR16ri %vreg0<tied0>, 2, %SRW<imp-def,dead>; GR16:%vreg1,%vreg0 # <-- This is the faulty instruction
        MOV16mr %noreg, <ga:@i>, %vreg1<kill>; mem:ST2[@i] GR16:%vreg1
        RET

[jobnoorman]

This is the same patch but based on current trunk.

[jobnoorman]

This patch should be committed in parallel with a similar patch for Clang. Can I just do that or should a create a diff for that one too?

[asl]

Go ahead and commit

[jobnoorman]

Thanks for the review!

LLVM: r218665
Clang: r218666

lib/Target/MSP430/MSP430Subtarget.cpp

MSP430Subtarget &MSP430Subtarget::initializeSubtargetDependencies(StringRef CPU, StringRef FS) {
   ParseSubtargetFeatures("generic", FS);
   return *this;
 }
 
 MSP430Subtarget::MSP430Subtarget(const std::string &TT, const std::string &CPU,
                                  const std::string &FS, const TargetMachine &TM)
     : MSP430GenSubtargetInfo(TT, CPU, FS),
       // FIXME: Check DataLayout string.
-      DL("e-m:e-p:16:16-i32:16:32-n8:16"), FrameLowering(),
+      DL("e-m:e-p:16:16-i32:16:32-a:16-n8:16"), FrameLowering(),
       InstrInfo(initializeSubtargetDependencies(CPU, FS)), TLInfo(TM),
       TSInfo(DL) {}