Page MenuHomePhabricator

[utils] Reflow asm check generation to tolerate blank lines
Needs ReviewPublic

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

Details

Summary

Rather than special casing asm check generation to simply output the check
lines, reflow the logic so that the IR and ASM check line generation use the
same mechanism for handling blank lines.

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.

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
216–217

if not is_asm:

239

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 TranscriptSun, Sep 1, 5:00 AM

There's CHECK-EMPTY

Even better - thanks!

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.