instruction 'align' in asm blocks works incorrectly with some cases of parameters

[rnk]

Summary

clean revision http://reviews.llvm.org/D14120
This align directive doesn't work on the -fasm-blocks.

ALIGN [[number]]
Aligns the next variable or instruction on a byte that is a multiple of number.

Microsoft support only power of 2 align like gcc and icc .
The problem with the code was in the AsmParser.cpp
Inside the AsmParser::parseDirectiveMSAlign function we trying to lowering number in to log of two. This is not correct and not consistent with Microsoft compilers.

align should work only with address of multiply of two.

int f(void)
{

__asm { align 2 }
__asm { align 4 }
__asm { align 8 }
__asm { align 16 }
__asm { align 32 }
return 0;

}

int main()
{

return f();

}

[rnk]

lib/MC/MCParser/AsmParser.cpp

4513

We don't need to pass IntValue here.

4715–4716

Val is unused now.

4718–4719

This comment is inaccurate now

4719

We don't need AdditionalSkip at all if you drop the trailing space on ".align ".

[rnk]

Actually, you need to check MCAsmInfo::getAlignmentIsInBytes() to decide if this rewrite is or isn't necessary. Some targets use log2 alignment values, and others use power of 2 alignment values.

lib/MC/MCParser/AsmParser.cpp

4679

This is now always zero, so you should zap it.

4679

Sorry, ignore this, I think we need to keep it along with the log2 machinery I asked you to remove.

4732

This addition is a no-op.

[m_zuckerman]

In D13869#271186, @rnk wrote:

Actually, you need to check MCAsmInfo::getAlignmentIsInBytes() to decide if this rewrite is or isn't necessary. Some targets use log2 alignment values, and others use power of 2 alignment values.

We don't need to check this because we are under the rule of Microsoft. This function operate only when ParsingInlineAsm is true. This is true only in Microsoft inline asm.

lib/MC/MCParser/AsmParser.cpp

4513

I tried to make as small changes as possible
But you are right :)

4679

We can keep or not the log2 machinery it's Does not matter. When we are here. we are under the rule of Microsoft. The rule of Microsoft is the value of the number. Numbers that have log2. Microsoft leaves this number as it is, and don't do anythink more.
Also the parseMSInlineAsm called only ones from Parser::ParseMicrosoftAsmStatement. This works when we under Microsft rule. So, the value is the inserted value. We don't need the extra AdditionalSkip.

[m_zuckerman]

lib/MC/MCParser/AsmParser.cpp

4679

For now the only set is here and is 0 . Maybe in future it will be used.

[rnk]

In D13869#271894, @m_zuckerman wrote:

We don't need to check this because we are under the rule of Microsoft. This function operate only when ParsingInlineAsm is true. This is true only in Microsoft inline asm.

Sure, ParsingInlineAsm is true, but there are actually multiple passes of assembly parsing. First we parse as intel style inline asm. This does rewrites as we go, to something that could be put in a .s file between .intel_syntax / .att_syntax directives. We take that result, reparse it using the regular asm parser, and then emit those MC instructions as either normal AT&T assembly or object code.

I claim you still need this rewrite on Darwin, which is not "under the rule of Microsoft" and alignment directives use log2 notation.

[m_zuckerman]

let say we running this test (I ran the test without any change on the code) .
-cc1 -triple i386-apple-darwin10 -fasm-blocks -mllvm --x86-asm-syntax=intel test.c -S -o test.s

TEST

int f(void)
{
    asm { align 2 }
    asm { align 4 }
    asm ( ".align 8 ");
    asm ( ".align 16" );

    return 0;
}

int main()
{
    return f();
}

Half in Microsoft style and half in Darwin/at&t.
Which of the syntax we will choose ?
I say that the programmer want to create a code in Microsoft style in the first two lines. This is the only way he can enter to the AsmParser::parseDirectiveMSAlign . He want it in Microsoft style not Darwin.
The last two lines are under Darwin path.

OUTPUT

1. .section TEXT,text,regular,pure_instructions
2. .macosx_version_min 10, 0
3. .intel_syntax noprefix
4. .globl _f
5. .align 4, 0x90
6. _f:
7. push eax
8. ## InlineAsm Start
10. .align 1, 0x90
12. ## InlineAsm End
13. mov dword ptr [esp], eax
14. ## InlineAsm Start
16. .align 2, 0x90
18. ## InlineAsm End
19. mov dword ptr [esp], eax
20. ## InlineAsm Start
21. .align 8, 0x90
22. ## InlineAsm End
23. ## InlineAsm Start
24. .align 16, 0x90
25. ## InlineAsm End
26. xor eax, eax
27. pop edx
28. ret
30. .globl _main
31. .align 4, 0x90
32. _main:
33. sub esp, 12
34. mov dword ptr [esp + 8], 0
35. call _f
36. add esp, 12
37. ret
38. .subsections_via_symbols

[rnk]

In your example, the user is writing MS-style inline asm, but the output of the compiler should be Darwin-style asm. The GNU-inline asm parts should be using log2 numbers on Darwin, not alignment-in-bytes numbers. The native Darwin assembler is going to interpret all .align directives as log2 values.

Do you see why we should make the log2 rewrite conditional on MCAsmInfo::getAlignmentIsInBytes()?

[m_zuckerman]

This is now the asm output of Darwin
-cc1 -triple i386-apple-darwin10 -fasm-blocks -mllvm --x86-asm-syntax=intel test.c -S -o test.s
Input:

int f(void)
{
    asm { align 2 }
    asm { align 4 }
    asm ( ".align 8 ");
    asm ( ".align 16" );

    return 0;
}

int main()
{
    return f();
}

Output:

        .section        __TEXT,__text,regular,pure_instructions
        .intel_syntax noprefix
        .intel_syntax noprefix
        .globl  _f
        .align  4, 0x90
_f:
        push    eax
        ## InlineAsm Start
 
        .align  2, 0x90
 
        ## InlineAsm End
        mov     dword ptr [esp], eax
        ## InlineAsm Start
 
        .align  4, 0x90
 
        ## InlineAsm End
        mov     dword ptr [esp], eax
        ## InlineAsm Start
        .align  8, 0x90
        ## InlineAsm End
        ## InlineAsm Start
        .align  16, 0x90
        ## InlineAsm End
        xor     eax, eax
        pop     edx
        ret
 
        .intel_syntax noprefix
        .globl  _main
        .align  4, 0x90
_main:
        sub     esp, 12
        mov     dword ptr [esp + 8], 0
        call    _f
        add     esp, 12
        ret

All the code is now under Darwin rule.

	INOUT	OUTPUT	Meaning
Microsoft	N=Number	.align N	Aligns the next variable or instruction on a byte that is a multiple of number.
Darwin	N = NUMBER 1<N<32	.align N	align_expression is a power of 2 (2^N)
Other Os Is In Bytes	N=Number	.align N	Aligns the next variable or instruction on a byte that is a multiple of number.

[rnk]

lib/MC/MCParser/AsmParser.cpp

4716

Most of your changes are unecessary. All you have to do is this change:

OS << ".align";
if (getContext().getAsmInfo()->getAlignmentIsInBytes())
  break;
OS << ' ';

test/CodeGen/ms-inline-asm.c

295–300 ↗

(On Diff #38379)

This test change is incorrect, as we want the previous results on Darwin.

I suggest adding a new test ms-inline-asm-align.c and testing Darwin and Windows targets.

[m_zuckerman]

lib/MC/MCParser/AsmParser.cpp

4517

For Microsoft and others OS that use power of two ,Like at&t. Need to rewrites align to -> .align

4518

InBytes true: For Microsoft and others OS that use power of two ,Like at&t. Need to check if the number is power of 2 and rewrites align to -> .align
InBytes false: Need to rewrites align to -> .align. and after Need to check if the number is under 32.

4520

Need to rewrites align to -> .align. Need to check if the number is under 32.

4715–4719

We need val for the assert.

4716

1. Actually we just need to do change from align to .align ,that all.

case AOK_Align: {
      OS << ".align";
}

2. We don't need the extra space.
3. We can check if the val < 32. (not must because there is another if test that doing that in parseDirectiveAlign).
4. see the table

	INOUT	OUTPUT	Meaning
Microsoft	N=Number	.align N	Aligns the next variable or instruction on a byte that is a multiple of number.
Darwin	N = NUMBER 1<N<32	.align N	align_expression is a power of 2 (2^N)
Other Os Is In Bytes	N=Number	.align N	Aligns the next variable or instruction on a byte that is a multiple of number.

All of the targets pass the value as it is. Except ,in the past Microsoft.
Some of them treat this number as power of 2 jump next byte.
Others treat as jump to the next byte that multiple of number. and this number is have log2.

4719

aling to .align

4720

We don't need additional skip. The value stay as it is.

test/CodeGen/ms-inline-asm.c

295–300 ↗

(On Diff #38379)

In D13869#273396, @rnk wrote:

In your example, the user is writing MS-style inline asm, but the output of the compiler should be Darwin-style asm. The GNU-inline asm parts should be using log2 numbers on Darwin, not alignment-in-bytes numbers. The native Darwin assembler is going to interpret all .align directives as log2 values.

Did you see how we configure the test?
// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fasm-blocks -emit-llvm -o - | FileCheck %s
We running on Darwin with fasm-blocks. The past result was incorrect. We need to fix the test.

you can find the definition in here
OS X Assembler Reference
Darwin pass the value as it is.

You can see it in other test also:

1. test\MC\AsmParser\comments-x86-darwin.s

// RUN: llvm-mc -triple x86_64-apple-darwin %s 2>&1 | FileCheck %s
# ensure that single '#' comments are worink as expected on x86 darwin
.align 3            # test single hash after align
// CHECK: .align 3
foo:                # single hash should be ignored as comment
// CHECK-LABEL: foo:
    movl %esp, %ebp # same after an instruction
// CHECK: movl %esp, %ebp
#   movl %esp, %ebp ## start of the line
// CHECK-NOT: movl %esp, %ebp
    # movl %esp, %ebp ## not quite start of the line
// CHECK-NOT: movl %esp, %ebp
bar:
// CHECK-LABEL: bar:

2. test\MC\AsmParser\directive_align.s

# RUN: not llvm-mc -triple i386-apple-darwin9 %s | FileCheck %s

# CHECK: TEST0:
# CHECK: .align 1
TEST0:  
        .align 1

# CHECK: TEST1:
# CHECK: .p2alignl 3, 0x0, 2
TEST1:  
        .align32 3,,2

# CHECK: TEST2:
# CHECK: .balign 3, 10
TEST2:  
        .balign 3,10

[m_zuckerman]

lib/MC/MCParser/AsmParser.cpp

4716

This path was correct for Microsoft in the past. And we can delete row number 4718 to 4720.

Actually we just need to do change from align to .align ,that all.

case AOK_Align: {
      OS << ".align";
      break;
}

2.We don't need the extra space .
3.We can check if the val < 32. (not must because there is another if test that doing that in parseDirectiveAlign so I agree with you about that).

All of the targets pass the value as it is. Except ,in the past Microsoft.
Some of them treat this number as power of 2 jump next byte.
Others treat as jump to the next byte that multiple of number.

[m_zuckerman]

lib/MC/MCParser/AsmParser.cpp

4720–4722

This is extra check. We don't must to doing that.

[rnk]

I'm testing a CL to handle this.

lib/MC/MCParser/AsmParser.cpp

bool AsmParser::parseDirectiveMSAlign(SMLoc IDLoc, ParseStatementInfo &Info) {
   const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(Value);
   if (!MCE)
     return Error(ExprLoc, "unexpected expression in align");
   uint64_t IntValue = MCE->getValue();
   if (!isPowerOf2_64(IntValue))
     return Error(ExprLoc, "literal value not a power of two greater then zero");
 
   Info.AsmRewrites->push_back(
-      AsmRewrite(AOK_Align, IDLoc, 5, Log2_64(IntValue)));
+      AsmRewrite(AOK_Align, IDLoc, 5, IntValue));
   return false;
 }
 
 // We are comparing pointers, but the pointers are relative to a single string.
 // Thus, this should always be deterministic.
 static int rewritesSort(const AsmRewrite *AsmRewriteA,
                         const AsmRewrite *AsmRewriteB) {
   if (AsmRewriteA->Loc.getPointer() < AsmRewriteB->Loc.getPointer())
     return -1;
   if (AsmRewriteB->Loc.getPointer() < AsmRewriteA->Loc.getPointer())
     return 1;
 
   // It's possible to have a SizeDirective, Imm/ImmPrefix and an Input/Output
   // rewrite to the same location.  Make sure the SizeDirective rewrite is
   // performed first, then the Imm/ImmPrefix and finally the Input/Output.  This
if (unsigned Len = Loc - AsmStart)
       OS << StringRef(AsmStart, Len);
 
     // Skip the original expression.
     if (Kind == AOK_Skip) {
       AsmStart = Loc + AR.Len;
       continue;
     }
 
     unsigned AdditionalSkip = 0;
     // Rewrite expressions in $N notation.
     switch (Kind) {
     default:
       break;
     case AOK_Imm:
       OS << "$$" << AR.Val;
       break;
     case AOK_ImmPrefix:
case AOK_SizeDirective:
       case 128: OS << "xmmword ptr "; break;
       case 256: OS << "ymmword ptr "; break;
       }
       break;
     case AOK_Emit:
       OS << ".byte";
       break;
     case AOK_Align: {
       unsigned Val = AR.Val;
-      OS << ".align " << Val;
+      OS << ".align " ;
       
       // Skip the original immediate.
-      assert(Val < 10 && "Expected alignment less then 2^10.");
-      AdditionalSkip = (Val < 4) ? 2 : Val < 7 ? 3 : 4;
+      AdditionalSkip = 1;
       break;
     }
     case AOK_DotOperator:
       // Insert the dot if the user omitted it.
       OS.flush();
       if (AsmStringIR.back() != '.')
         OS << '.';
       OS << AR.Val;
       break;
     }
 
     // Skip the original expression.
     AsmStart = Loc + AR.Len + AdditionalSkip;
   }
 
   // Emit the remainder of the asm string.
   if (AsmStart != AsmEnd)
     OS << StringRef(AsmStart, AsmEnd - AsmStart);
 
   AsmString = OS.str();
   return false;