[PATCH] D122986: [UpdateTestChecks] Add NVPTX support in update_llc_test_checks.py

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/utils/UpdateTestChecks/asm.py:460
+      'csky': (scrub_asm_csky, ASM_FUNCTION_CSKY_RE, ':'),
+      'nvptx': (scrub_asm_nvptx, ASM_FUNCTION_NVPTX_RE, '(')
   }
----------------
kovdan01 wrote:
> tra wrote:
> > 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.
> > 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.
> > 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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122986/new/

https://reviews.llvm.org/D122986