Re: [PATCH] D12399: Microsoft compatibility – add support for “relaxation” of memory operands in inline assembly.

Reid Kleckner via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 27 09:14:00 PDT 2015


rnk added a comment.

There are more than a few spelling mistakes in this patch, so I'd appreciate it if you ran spellcheck first. Also consider reformatting the C++ code with clang-format. The instructions for using it on your patch are here: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2672
@@ +2671,3 @@
+  // This will work only with triple -x86-64-pc-windows-msvc
+  bool RelaxcationMode = STI.getTargetTriple().getOS() == STI.getTargetTriple().Win32; 
+  unsigned M = MatchInstructionImpl(Operands, MInst, ErrorInfoIgnore,
----------------
I think the right condition here is not "are we targeting Windows" but "are we parsing Intel inline assembly".

================
Comment at: test/MC/X86/relaxation.s:1
@@ +1,2 @@
+// RUN:%llvm-mc -triple x86_64-pc-windows-msvc -x86-asm-syntax=intel -output-asm-variant=1 -S -o - %s | FileCheck %s
+/*
----------------
Given my comment above about making this conditional on inline assembly, you will have to make this a clang test for now.

================
Comment at: test/MC/X86/relaxation.s:2-7
@@ +1,8 @@
+// RUN:%llvm-mc -triple x86_64-pc-windows-msvc -x86-asm-syntax=intel -output-asm-variant=1 -S -o - %s | FileCheck %s
+/*
+ This file is auto-generated to test inline assembler insertion compilation
+ This is test for instruction 'not'. This test must pass.
+ IMPORTANT:
+ use just to compile, NOT to run
+*/
+
----------------
This comment isn't needed, this test isn't different from the other tests in this directory.


http://reviews.llvm.org/D12399





More information about the llvm-commits mailing list