[PATCH] D13869: instruction 'align' in asm blocks works incorrectly with some cases of parameters

[michael zuckerman via llvm-commits]

m_zuckerman added inline comments.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4512
@@ +4511,3 @@
+      return Error(ExprLoc, "literal value not a power of two greater then zero");
+    Info.AsmRewrites->emplace_back(AOK_Align, IDLoc, 5);
+  }
----------------
For Microsoft and others OS that use power of two ,Like at&t. Need to rewrites align to -> .align 

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4515
@@ -4514,1 +4514,3 @@
+  else
+    Info.AsmRewrites->emplace_back(AOK_Align, IDLoc, 5, IntValue);
   return false;
----------------
Need to rewrites align to -> .align. Need to check if the number is under 32.  


================
Comment at: lib/MC/MCParser/AsmParser.cpp:4716
@@ -4714,2 +4715,3 @@
     case AOK_Align: {
       unsigned Val = AR.Val;
+      OS << ".align";
----------------
We need val for the assert. 

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4717
@@ -4715,6 +4716,3 @@
       unsigned Val = AR.Val;
-      OS << ".align " << Val;
-
-      // Skip the original immediate.
-      assert(Val < 10 && "Expected alignment less then 2^10.");
-      AdditionalSkip = (Val < 4) ? 2 : Val < 7 ? 3 : 4;
+      OS << ".align";
+      if (!getContext().getAsmInfo()->getAlignmentIsInBytes()){
----------------
aling to .align


================
Comment at: lib/MC/MCParser/AsmParser.cpp:4513
@@ -4514,1 +4512,3 @@
+  }
+    Info.AsmRewrites->emplace_back(AOK_Align, IDLoc, 5, IntValue);
   return false;
----------------
**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.


================
Comment at: lib/MC/MCParser/AsmParser.cpp:4716
@@ -4715,3 +4714,1 @@
       unsigned Val = AR.Val;
-      OS << ".align " << Val;
-
----------------
rnk wrote:
> Most of your changes are unecessary. All you have to do is this change:
>   OS << ".align";
>   if (getContext().getAsmInfo()->getAlignmentIsInBytes())
>     break;
>   OS << ' ';
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.



================
Comment at: lib/MC/MCParser/AsmParser.cpp:4720
@@ -4719,3 +4719,1 @@
-      assert(Val < 10 && "Expected alignment less then 2^10.");
-      AdditionalSkip = (Val < 4) ? 2 : Val < 7 ? 3 : 4;
       break;
----------------
We don't need additional skip. The value stay as it is. 

================
Comment at: test/CodeGen/ms-inline-asm.c:295-300
@@ -300,2 +294,8 @@
 // CHECK: .align 8
+  __asm align 16;
+// CHECK: .align 16
+  __asm align 20;
+// CHECK: .align 20
+  __asm ALIGN 31;
+// CHECK: .align 31
 // CHECK: "~{dirflag},~{fpsr},~{flags}"()
----------------
rnk wrote:
> 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.
>>! 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 
[[ https://developer.apple.com/library/mac/documentation/DeveloperTools/Reference/Assembler/Assembler.pdf| 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
```



http://reviews.llvm.org/D13869