[UpdateTestChecks] Add support for SPIRV in update_llc_test_checks.py #66213
Conversation
|
@llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-testing-tools ChangesWe have to change the location of the comment for function start in SPIRV. We add a new command line flag --skip-check-label, because there's no function label on SPIRV.Previously https://reviews.llvm.org/D157858Full diff: https://github.com/llvm/llvm-project/pull/66213.diff 3 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp b/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
index 27da0f21f1571d3..bc0cd64b0b4d154 100644
--- a/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
@@ -118,6 +118,7 @@ void SPIRVAsmPrinter::emitFunctionHeader() {
OutStreamer->getCommentOS()
<< "-- Begin function "
<< GlobalValue::dropLLVMManglingEscape(F.getName()) << '\n';
+ OutStreamer->addBlankLine();
}
auto Section = getObjFileLowering().SectionForGlobal(&F, TM);
diff --git a/llvm/utils/UpdateTestChecks/asm.py b/llvm/utils/UpdateTestChecks/asm.py
index 33579065132179c..ea823d766469aeb 100644
--- a/llvm/utils/UpdateTestChecks/asm.py
+++ b/llvm/utils/UpdateTestChecks/asm.py
@@ -15,6 +15,11 @@ class string:
# RegEx: this is where the magic happens.
##### Assembly parser
+#
+# The set of per-arch regular expressions define several groups.
+# The required groups are "func" (function name) and "body" (body of the function).
+# Although some backends require some additional groups like: "directives"
+# and "func_name_separator"
ASM_FUNCTION_X86_RE = re.compile(
r'^_?(?P<func>[^:]+):[ \t]*#+[ \t]*(@"?(?P=func)"?| -- Begin function (?P=func))\n(?:\s*\.?Lfunc_begin[^:\n]*:\n)?'
@@ -197,6 +202,18 @@ class string:
flags=(re.M | re.S),
)
+# We parse the function name from the comments issued by the backend:
+# ; -- Begin function <name>
+# and then the final comment
+# ; -- End function
+# If these change in the future, we need to change the regex.
+ASM_FUNCTION_SPIRV_RE = re.compile(
+ r'[ \t]+; \-\- Begin function (?P<func>[^\n]+)\n'
+ r"(?P<body>.*?)\n"
+ r"[ \t]+; \-\- End function",
+ flags=(re.M | re.S),
+)
+
ASM_FUNCTION_VE_RE = re.compile(
r"^_?(?P<func>[^:]+):[ \t]*#+[ \t]*@(?P=func)\n"
r"(?:\s*\.?L(?P=func)\$local:\n)?" # optional .L<func>$local: due to -fno-semantic-interposition
@@ -433,6 +450,17 @@ def scrub_asm_sparc(asm, args):
return asm
+def scrub_asm_spirv(asm, args):
+ # Scrub runs of whitespace out of the assembly, but leave the leading
+ # whitespace in place.
+ asm = common.SCRUB_WHITESPACE_RE.sub(r" ", asm)
+ # Expand the tabs used for indentation.
+ asm = string.expandtabs(asm, 2)
+ # Strip trailing whitespace.
+ asm = common.SCRUB_TRAILING_WHITESPACE_RE.sub(r"", asm)
+ return asm
+
+
def scrub_asm_systemz(asm, args):
# Scrub runs of whitespace out of the assembly, but leave the leading
# whitespace in place.
@@ -547,6 +575,8 @@ def get_run_handler(triple):
"riscv64": (scrub_asm_riscv, ASM_FUNCTION_RISCV_RE),
"lanai": (scrub_asm_lanai, ASM_FUNCTION_LANAI_RE),
"sparc": (scrub_asm_sparc, ASM_FUNCTION_SPARC_RE),
+ "spirv32": (scrub_asm_spirv, ASM_FUNCTION_SPIRV_RE),
+ "spirv64": (scrub_asm_spirv, ASM_FUNCTION_SPIRV_RE),
"s390x": (scrub_asm_systemz, ASM_FUNCTION_SYSTEMZ_RE),
"wasm32": (scrub_asm_wasm, ASM_FUNCTION_WASM_RE),
"wasm64": (scrub_asm_wasm, ASM_FUNCTION_WASM_RE),
diff --git a/llvm/utils/UpdateTestChecks/common.py b/llvm/utils/UpdateTestChecks/common.py
index 22c05322d9c7dbc..7d2c065cf669090 100644
--- a/llvm/utils/UpdateTestChecks/common.py
+++ b/llvm/utils/UpdateTestChecks/common.py
@@ -196,6 +196,13 @@ def __call__(self, parser, namespace, values, option_string=None):
default=[],
help="List of regular expressions such that, for matching global value declarations, literal integer values should be encoded in hex in the associated FileCheck directives",
)
+ parser.add_argument(
+ "--skip-check-label",
+ action="store_true",
+ dest="skip_check_label",
+ default=False,
+ help="Activate CHECK-LABEL line generation per function",
+ )
# FIXME: in 3.9, we can use argparse.BooleanOptionalAction. At that point,
# we need to rename the flag to just -generate-body-for-unused-prefixes.
parser.add_argument(
@@ -212,10 +219,11 @@ def __call__(self, parser, namespace, values, option_string=None):
)
args = parser.parse_args()
# TODO: This should not be handled differently from the other options
- global _verbose, _global_value_regex, _global_hex_value_regex
+ global _verbose, _global_value_regex, _global_hex_value_regex, _skip_check_label
_verbose = args.verbose
_global_value_regex = args.global_value_regex
_global_hex_value_regex = args.global_hex_value_regex
+ _skip_check_label = args.skip_check_label
return args
@@ -1306,7 +1314,7 @@ def add_checks(
output_lines.append(
"%s %s-SAME: %s" % (comment_marker, checkprefix, args_and_sig)
)
- else:
+ elif not _skip_check_label:
output_lines.append(
check_label_format
% (
|
|
Could we just emit a different label format that matches the comment for spirv? Having to pass the flag seems a bit awkward |
I would love to be able to do that but the Happy to hear any ideas that might ease changing this without breaking other stuff. |
pmatos
force-pushed
the
UpdateTestChecks-SPIRV
branch
from
ac65b0f
to
01332f5
Compare
|
How different would check_label_format have to be? Surely SPIRV has something similar that we can use? I always like to see at least one use of the update script when we add a new target - please can you regenerate a suitable file in CodeGen\SPIRV to show the generated checks? |
pmatos
force-pushed
the
UpdateTestChecks-SPIRV
branch
from
01332f5
to
308d1d4
Compare
The assembly for the test I just committed looks like: Besides the comment there's no obvious place where the function body starts and its name due to the structure of the SPIRV file.
Now updated the commit to include one of these. |
|
Why not just match OpName ? ASM_FUNCTION_X86_RE does something similar in matching the func name a few lines before the "Begin function" tag |
As far as I understand we need to grab the body of the function. In the case of SPIRV, the body doesn't come after the name. Take the case of 2 functions called void1 and void2: How would you grab with a regex the body of void2 if I parse the name from OpName? |
How about something like something along the line of: |
|
Maybe something like |
Thanks, I have updated the patch with a regex based on your suggestion that seems to work well. |
To be honest, I don't understand the suggestion. I removed - for explanatory purposes - the flag from the commit and regenerated the test. Now the check lines look like: The problem is the *-LABEL line which is not issues at all by SPIRV. It seems you know how to skip this line without having an extra flag but I don't understand what you mean by:
By guess at an answer is that this is still not sufficient because there's no label at all to match. |
The ':' is added because that's the default value of |
Ah yes, I understand what you are saying and I see you have in your suggested RE you have but SPIRV issues no labels. The odd thing (for me) is that:
|
That's just a sideeffect of some of the prefixes being manually added |
Thanks - I will merge this then. |
We have to change the location of the comment for function start in SPIRV. We add a new command line flag --skip-check-label, because there's no function label on SPIRV. Previously https://reviews.llvm.org/D157858
Simplify regexp based on suggestions and update testcase (now broken).
pmatos
force-pushed
the
UpdateTestChecks-SPIRV
branch
from
8ebf04f
to
02787fa
Compare
We have to change the location of the comment for function start
in SPIRV. We add a new command line flag --skip-check-label, because
there's no function label on SPIRV.
Previously https://reviews.llvm.org/D157858