[PATCH] D125789: Fix release note typo from 6da3d66f

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc created this revision.
Herald added subscribers: pengfei, hiraditya.
Herald added a project: All.
sheisc requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

The GNU assembler (used in gcc) complains about the syntactic correctness of
the assembly code (at line 9) generated by clang (with the option 
-no-integrated-as).

We need to delete the white space in "{%k1} {z}" (at line 9) to make both GCC 
and LLVM happy.

iron@CSE:demo$ cat -n main.s

   1.text
   2.file   "main.c"
   3.globl  main# -- Begin function main
   4.p2align4, 0x90
   5.type   main,@function
   6main:   # @main
   7.cfi_startproc
   8# %bb.0:# %entry
   9vmovdqu16 %zmm5 , %zmm5 {%k1} {z}
  10xorl%eax, %eax
  11retq
  12.Lfunc_end0:
  13.size   main, .Lfunc_end0-main
  14.cfi_endproc
  15# -- End function
  16
  17.ident  "clang"
  18.section".note.GNU-stack","",@progbits

iron@CSE:demo$ clang -c main.s -o main.o

iron@CSE:demo$ gcc -c main.s -o main.o

main.s: Assembler messages:
main.s:9: Error: unknown vector operation: ` {z}'


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125789

Files:
  clang/docs/ReleaseNotes.rst
  llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
  llvm/lib/Target/X86/X86MCInstLower.cpp


Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++ llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1877,7 +1877,7 @@
   CS << " {%" << GetRegisterName(WriteMaskOp.getReg()) << "}";
 
   if (SrcOp1Idx == 2) {
-CS << " {z}";
+CS << "{z}";
   }
 }
   }
Index: llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
@@ -278,7 +278,7 @@
 
   // MASKZ: zmmX {%kY} {z}
   if (MaskWithZero)
-OS << " {z}";
+OS << "{z}";
 }
 
 static bool printFMAComments(const MCInst *MI, raw_ostream &OS,
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -365,7 +365,7 @@
   of clang; use the ``-fclang-abi-compat=14`` option to get the old mangling.
 - Preprocessor character literals with a ``u8`` prefix are now correctly 
treated as
   unsigned character literals. This fixes `Issue 54886 
`_.
-- Stopped allowing constriants on non-template functions to be compliant with
+- Stopped allowing constraints on non-template functions to be compliant with
   dcl.decl.general p4.
 
 C++20 Feature Support


Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++ llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1877,7 +1877,7 @@
   CS << " {%" << GetRegisterName(WriteMaskOp.getReg()) << "}";
 
   if (SrcOp1Idx == 2) {
-CS << " {z}";
+CS << "{z}";
   }
 }
   }
Index: llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
@@ -278,7 +278,7 @@
 
   // MASKZ: zmmX {%kY} {z}
   if (MaskWithZero)
-OS << " {z}";
+OS << "{z}";
 }
 
 static bool printFMAComments(const MCInst *MI, raw_ostream &OS,
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -365,7 +365,7 @@
   of clang; use the ``-fclang-abi-compat=14`` option to get the old mangling.
 - Preprocessor character literals with a ``u8`` prefix are now correctly treated as
   unsigned character literals. This fixes `Issue 54886 `_.
-- Stopped allowing constriants on non-template functions to be compliant with
+- Stopped allowing constraints on non-template functions to be compliant with
   dcl.decl.general p4.
 
 C++20 Feature Support
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125789: FIX the assembly format of the x86 backend to make both clang and gcc happy

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc updated this revision to Diff 430067.
sheisc retitled this revision from "Fix release note typo from 6da3d66f" to 
"FIX the assembly format of the x86 backend to make both clang and gcc happy".
sheisc edited the summary of this revision.
sheisc added a project: libc-project.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125789/new/

https://reviews.llvm.org/D125789

Files:
  llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
  llvm/lib/Target/X86/X86MCInstLower.cpp


Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++ llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1877,7 +1877,7 @@
   CS << " {%" << GetRegisterName(WriteMaskOp.getReg()) << "}";
 
   if (SrcOp1Idx == 2) {
-CS << " {z}";
+CS << "{z}";
   }
 }
   }
Index: llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
@@ -278,7 +278,7 @@
 
   // MASKZ: zmmX {%kY} {z}
   if (MaskWithZero)
-OS << " {z}";
+OS << "{z}";
 }
 
 static bool printFMAComments(const MCInst *MI, raw_ostream &OS,


Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++ llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1877,7 +1877,7 @@
   CS << " {%" << GetRegisterName(WriteMaskOp.getReg()) << "}";
 
   if (SrcOp1Idx == 2) {
-CS << " {z}";
+CS << "{z}";
   }
 }
   }
Index: llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp
@@ -278,7 +278,7 @@
 
   // MASKZ: zmmX {%kY} {z}
   if (MaskWithZero)
-OS << " {z}";
+OS << "{z}";
 }
 
 static bool printFMAComments(const MCInst *MI, raw_ostream &OS,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125789: FIX the assembly format of the x86 backend to make both clang and gcc happy

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc abandoned this revision.
sheisc added a comment.

In D125789#3519361 , @pengfei wrote:

> I guess a lot of lines of tests need to update
>
>   $ grep -rn " {z}" llvm/test/CodeGen/X86/ | wc -l
>   7797

Thanks.
Yes. It seems to be a big revision, caused by only one white space :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125789/new/

https://reviews.llvm.org/D125789

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125789: FIX the assembly format of the x86 backend to make both clang and gcc happy

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc added a comment.

In D125789#3519411 , @pengfei wrote:

> I think another way is to report the issue to GCC. From the perspective of 
> the user, GCC should support both `{%k1} {z}` and `{%k1}{z}`. Then we don't 
> need the clange on LLVM.

Yes. It is a good idea. 
However, it appears that there is no such white space in the instructions as 
described in Intel's manuals.
So I don't know which one should be the correct format.
Anyway, not a big issue.
I found this problem when using the fuzzer (i.e. AFL) to build Firefox.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125789/new/

https://reviews.llvm.org/D125789

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125789: FIX the assembly format of the x86 backend to make both clang and gcc happy

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc added a comment.

In D125789#3519493 , @pengfei wrote:

> In D125789#3519433 , @sheisc wrote:
>
>> In D125789#3519411 , @pengfei 
>> wrote:
>>
>>> I think another way is to report the issue to GCC. From the perspective of 
>>> the user, GCC should support both `{%k1} {z}` and `{%k1}{z}`. Then we don't 
>>> need the clange on LLVM.
>>
>> Yes. It is a good idea. 
>> However, it appears that there is no such white space in the instructions as 
>> described in Intel's manuals.
>> So I don't know which one should be the correct format.
>> Anyway, not a big issue.
>> I found this problem when using the fuzzer (i.e. AFL) to build Firefox.
>
> Yeah. This is an interesting question. I didn't notice the difference between 
> LLVM and GCC. I think either way changing here or GCC is OK :)

Hi Pengfei,

Thanks a lot.  The guys in the GCC community can make our life easier by 
supporting the white space.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125789/new/

https://reviews.llvm.org/D125789

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125789: FIX the assembly format of the x86 backend to make both clang and gcc happy

2022-05-17 Thread Changwei Zou via Phabricator via cfe-commits 
sheisc added a comment.

In D125789#3519538 , @craig.topper 
wrote:

> Is it gcc that needs a fix or binutils?

Yes. Exactly speaking, it is the GNU assembler in binutils.
https://www.gnu.org/software/binutils/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125789/new/

https://reviews.llvm.org/D125789

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits