This is an archive of the discontinued LLVM Phabricator instance.

[UpdateTestChecks] Add NVPTX support in update_llc_test_checks.py
ClosedPublic

Authored by kovdan01 on Apr 2 2022, 1:53 PM.

Details

Summary

This patch makes possible generating NVPTX assembly check lines with
update_llc_test_checks.py utility.

Diff Detail

Event Timeline

kovdan01 created this revision.Apr 2 2022, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2022, 1:53 PM
kovdan01 requested review of this revision.Apr 2 2022, 1:53 PM
tra added a subscriber: tra.Apr 4 2022, 12:18 PM
tra added inline comments.
llvm/utils/UpdateTestChecks/asm.py
184

There may be other attributes (e.g. .weak) and they may come in different order.
It may be better to use something like (\.(func|visible|weak|entry|noreturn|extern)\s+).

430

It would be great to document what the function is expected to return.

503

The third field is a label_suffix, but it's not clear why it's ( for NVPTX.
AFAICT, PTX does use ':' for the labels: https://cuda.godbolt.org/z/34185xEbY

Looks like it's supposed to serve as a terminator for the CHECK-LABEL (i.e. it's a suffix in terms of FileCheck, but not necessarily in terms of the actual asm labels).
This may be something to add to the description of the function return value.

I wonder if we can incorporate this suffix into a named regex match group, so we don't have to pass it explicitly.

kovdan01 added inline comments.Apr 5 2022, 9:04 AM
llvm/utils/UpdateTestChecks/asm.py
503

The third field is a label_suffix, but it's not clear why it's ( for NVPTX.
AFAICT, PTX does use ':' for the labels: https://cuda.godbolt.org/z/34185xEbY

I suppose that I used a wrong term for naming that, and maybe I should rename that to function_decl_suffix or something like this.
Maybe I have missed something, but as far as I understood functions in PTX begin like this - having ( after function name:

.visible .entry square(int *, int)(
        .param .u64 square(int *, int)_param_0,
        .param .u32 square(int *, int)_param_1
)

In other assemblies, function "declarations" are just labels looking like foo:. So I introduced this new suffix field, because we need to have some marker of function start. Previously, a colon after label was hardcoded.

I wonder if we can incorporate this suffix into a named regex match group, so we don't have to pass it explicitly.

Unfortunately, that seems to be even more non-transparent - this was the first way that I tried and failed. Regex named group func is intended to hold the same function name as IR function has. Here is code from add_checks function from common.py:

# If we do not have output for this prefix we skip it.
if not func_dict[checkprefix][func_name]:
  continue

I also don't like that separate suffix field, but have no idea how to get rid of it in a nice way.

tra added inline comments.Apr 5 2022, 11:21 AM
llvm/utils/UpdateTestChecks/asm.py
503

I wonder if we can incorporate this suffix into a named regex match group, so we don't have to pass it explicitly.

Unfortunately, that seems to be even more non-transparent - this was the first way that I tried and failed. Regex named group func is intended to hold the same function name as IR function has. Here is code from add_checks function from common.py:

# If we do not have output for this prefix we skip it.
if not func_dict[checkprefix][func_name]:
  continue

I was thinking of adding the separator as yet another named group within the existing RE for function parameters:
r'\([^\)]*\)(\s*//[^\n]*)?\n' -> r'(?P<separator>\()[^\)]*\)(\s*//[^\n]*)?\n'

The named group 'func' remains unchanged, but now you can extract the separator via its own named group, save it along with the function name and later retrieve and use it in add_checks.

To minimize churn, the separator group may be optional and would default to ':' which would work for existing targets.

kovdan01 updated this revision to Diff 421192.Apr 7 2022, 7:00 AM
kovdan01 added a reviewer: tra.
tra added a comment.Apr 7 2022, 11:22 AM

LGTM, but I'll defer to UpdateTestChecks owners for the final approval.

llvm/utils/UpdateTestChecks/common.py
493–497

Existence of the named group can be checked directly:

func_name_separator = m.group('func_name_separator')  if 'func_name_separator' in m.groupdict() else ":"
kovdan01 updated this revision to Diff 421312.Apr 7 2022, 12:25 PM
kovdan01 added reviewers: Flakebi, jdoerfert, pengfei.
kovdan01 marked an inline comment as done.
kovdan01 added inline comments.
llvm/utils/UpdateTestChecks/common.py
493–497

Thanks for useful advice! Fixed.

kovdan01 marked 5 inline comments as done.Apr 7 2022, 12:26 PM

@lattner Hello Chris, could you please suggest a reviewer for this patch? I'm not sure who is the owner of this part of repository. Thanks!

I don't know who would be the best to review this, @MaskRay do you know?

lattner removed a subscriber: lattner.Apr 11 2022, 10:34 PM
MaskRay accepted this revision.Apr 11 2022, 10:48 PM
MaskRay added inline comments.
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/nvptx-basic.ll
22

nit: keep just one blank line

43

nit: delete trailing blank line

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/nvptx-basic.ll.expected
82

opaque pointers have been enabled today.
You may need to switch to ptr

This revision is now accepted and ready to land.Apr 11 2022, 10:48 PM
kovdan01 updated this revision to Diff 423037.Apr 15 2022, 1:01 AM
kovdan01 marked 3 inline comments as done.Apr 15 2022, 1:01 AM
kovdan01 added inline comments.
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/nvptx-basic.ll.expected
82

Fixed, thanks!

This revision was landed with ongoing or failed builds.Apr 15 2022, 1:02 AM
This revision was automatically updated to reflect the committed changes.
kovdan01 marked an inline comment as done.