Page MenuHomePhabricator

[Utils] Do not remove comments in llc test script
Needs ReviewPublic

Authored by sebastian-ne on Apr 26 2021, 11:03 AM.

Details

Summary

When checking if two prefixes can be merged for a function,
update_llc_test_checks.py removed IR comments before comparing
llc outputs of different RUN lines.
This means, if one RUN line emited lines starting with ';' and another
RUN line emited the same lines except the ones starting with ';', both
RUNs would be merged (if they share a prefix).

However, CHECK-NEXT lines check the comments, otherwise they fail, so
the script should not merge RUNs if they contain different comments.

Diff Detail

Event Timeline

sebastian-ne created this revision.Apr 26 2021, 11:03 AM
sebastian-ne requested review of this revision.Apr 26 2021, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 11:03 AM

I'm trying to understand the functionality you want to achieve. Doesn't using different prefixes avoid merging already? The test does not seem to exercise your extensions.

I'm trying to understand the functionality you want to achieve. Doesn't using different prefixes avoid merging already? The test does not seem to exercise your extensions.

He, that is a good point, it was my intention to add a common prefix to the test. I added that and checked that the test now fails without the script changes.

(It will end up merging the output for the two run lines. Running FileCheck will then fail for the GFX9-O0 case, because the comments are missing in the CHECK-NEXT lines.)

friendly ping for review

foad added inline comments.May 7 2021, 2:37 AM
llvm/utils/UpdateTestChecks/common.py
265

It seems like most of the "arg name scrubbing" that this function does is not relevant for asm? Perhaps update_llc_test_checks could use a much much simpler form of this function instead?

foad added a comment.May 7 2021, 2:46 AM

Also, I don't understand why it would be OK for non-asm dialects to remove a whole-line comment. Maybe the problem is that SCRUB_IR_COMMENT should remove a trailing comment only, but not completely delete a whole-line comment?

I guess it is fine for MIR tests because they only use CHECK lines instead of CHECK-NEXT lines.

foad added a comment.May 7 2021, 3:10 AM

I guess it is fine for MIR tests because they only use CHECK lines instead of CHECK-NEXT lines.

In add_checks() it looks like non-asm dialects use CHECK-NEXT normally, but CHECK after one or more blank lines.