This is an archive of the discontinued LLVM Phabricator instance.

[utils] Reflow asm check generation to tolerate blank lines
ClosedPublic

Authored by atanasyan on May 22 2018, 3:11 AM.

Details

Summary

This change introduces two fixes. The second fix allows to generate a test to check the first fix.

  • Output CHECK-EMPTY prefix for an empty line in ASM output. Before that fix update_llc_test_checks.py incorrectly emits CHECK-NEXT: <space> prefix.
  • Fix the ASM_FUNCTION_MIPS_RE regex to stop on a real function epilogue not on an inline assembler prologue and include inline assembler code into a test.

Diff Detail

Event Timeline

sdardis created this revision.May 22 2018, 3:11 AM
MaskRay added inline comments.May 22 2018, 12:31 PM
utils/UpdateTestChecks/common.py
195 ↗(On Diff #147976)

if not is_asm:

218 ↗(On Diff #147976)

if not is_asm:

sdardis updated this revision to Diff 149268.May 31 2018, 5:33 AM

Address review comments.

This change enables the update_llc_test_checks.py utility to correctly generate
CHECK lines for inline assembly, where the llc can insert blank lines before
and after an inline assembly string.

Was that not possible before? Perhaps you can showcase that here?

In D48006, you can see the change for the MIPS specific assembly scrubbing to accept inline assembly. I have also included some example output of the changes of that revision. Since the blank lines around the inline assembly are not scrubbed, FileCheck fails the test case with:

/Users/sdardis/dev/llvm/llvm/test/CodeGen/Mips/cconv/arguments-varargs.ll:33:16: error: found empty check string with prefix 'O32-BE:'
; O32-BE-NEXT: 
               ^
LLVM ERROR: IO failure on output stream: Broken pipe

With D48006 applied, then applying this diff and regenerating test/CodeGen/Mips/cconv/arguments-varargs.ll we can see the following set of changes:

diff --git a/test/CodeGen/Mips/cconv/arguments-varargs.ll b/test/CodeGen/Mips/cconv/arguments-varargs.ll
index 7739ca2..3277a6e 100644
--- a/test/CodeGen/Mips/cconv/arguments-varargs.ll
+++ b/test/CodeGen/Mips/cconv/arguments-varargs.ll
@@ -30,10 +30,8 @@ define void @fn_i16_dotdotdot_i16(i16 %a, ...) {
 ; O32-BE-NEXT:    .set at
 ; O32-BE-NEXT:    .set macro
 ; O32-BE-NEXT:    .set reorder
-; O32-BE-NEXT:  
-; O32-BE-NEXT:    teqi $zero, 1
-; O32-BE-NEXT:  
-; O32-BE-NEXT:    .set pop
+; O32-BE:         teqi $zero, 1
+; O32-BE:         .set pop
 ; O32-BE-NEXT:    #NO_APP
 ; O32-BE-NEXT:    lw $1, 0($sp)
 ; O32-BE-NEXT:    addiu $2, $1, 4

(There are more occurrences like this.) By scrubbing out the blank lines and restarting the LABEL/LABEL-NEXT checks as appropriate, the test case is correctly regenerated and includes all the inline assembly.

sdardis updated this revision to Diff 150702.Jun 11 2018, 3:45 AM

Rebase to master.

Does the following patch uses new CHECK-EMPTY directive solves the problem?

--- a/utils/UpdateTestChecks/common.py
+++ b/utils/UpdateTestChecks/common.py
@@ -215,7 +215,10 @@ def add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
       if is_asm == True:
         output_lines.append('%s %s:       %s' % (comment_marker, checkprefix, func_body[0]))
         for func_line in func_body[1:]:
-          output_lines.append('%s %s-NEXT:  %s' % (comment_marker, checkprefix, func_line))
+          if func_line.strip() == '':
+            output_lines.append('%s %s-EMPTY:' % (comment_marker, checkprefix))
+          else:
+            output_lines.append('%s %s-NEXT:  %s' % (comment_marker, checkprefix, func_line))
         break

       # For IR output, change all defs to FileCheck variables, so we're immune

The tests were fixed in rL343730.

I'm not sure if its a good idea to start skipping lines like this - would a fix to make FileCheck more tolerant of blank lines be any better?

Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2019, 5:00 AM

What do you think about teaching UpdateTestChecks to generate CHECK-EMPTY when a line is empty?

Does the following patch uses new CHECK-EMPTY directive solves the problem?

--- a/utils/UpdateTestChecks/common.py
+++ b/utils/UpdateTestChecks/common.py
@@ -215,7 +215,10 @@ def add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
       if is_asm == True:
         output_lines.append('%s %s:       %s' % (comment_marker, checkprefix, func_body[0]))
         for func_line in func_body[1:]:
-          output_lines.append('%s %s-NEXT:  %s' % (comment_marker, checkprefix, func_line))
+          if func_line.strip() == '':
+            output_lines.append('%s %s-EMPTY:' % (comment_marker, checkprefix))
+          else:
+            output_lines.append('%s %s-NEXT:  %s' % (comment_marker, checkprefix, func_line))
         break

       # For IR output, change all defs to FileCheck variables, so we're immune

What do you think about teaching UpdateTestChecks to generate CHECK-EMPTY when a line is empty?

Yes - sorry I blanked that snippet for some reason. Using CHECK-EMPTY and including a test change in this patch to show the diff would be good.

reverse ping

atanasyan commandeered this revision.Oct 30 2019, 10:10 AM
atanasyan added a reviewer: sdardis.
atanasyan edited the summary of this revision. (Show Details)
  • Emit CHECK-EMPTY prefix for an empty line in ASM output
  • Add MIPS related fix and update test case to check the changes.
RKSimon accepted this revision.Oct 30 2019, 4:09 PM

LGTM - cheers

This revision is now accepted and ready to land.Oct 30 2019, 4:09 PM
This revision was automatically updated to reflect the committed changes.