[RFC] update_llc_test_checks: Support downstream targets with a new option pointing to downstream handlers

LLVM Project
1

Hi everyone,

I am trying to add support for a downstream target in update_llc_test_checks.py and I am considering my options. I think one of the better ones is to extend the script with a flag --downstream-target-handler-path <downstream_handlers.py> pointing to a downstream implementation of the handlers (see draft patch), but I would like to hear your thoughts about it.

Thanks,
Vasileios

About

Supporting update_llc_test_checks.py for a downstream target can be done in two ways:

  1. Creating and maintaining a downstream copy of the update_llc_test_checks.py script, or
  2. Patching the upstream version and keep applying the patch on every pull-down.

Neither is great. The first one will eventually go out of sync with upstream and the second one adds overhead to the pull-down process, and it also increases the chances of having to resolve conflicts.

This RFC is about adding a --downstream-target-handler-path option to update_llc_test_checks.py pointing to a downstream python module that implements the necessary target handler functions.

Proposal

Introduce a new option: update_llc_test_checks.py --downstream-target-handler-path <path to downstream module>

The <downstream module> can live downstream and needs to implement the following functions:

def scrub(asm, args):

def get_run_handler(triple):

def add_checks(...):

The downstream module is loaded by update_llc_test_checks.py and the handlers defined in it are used instead of the ones in [asm.py](http://asm.py) or [isel.py](http://isel.py).

A draft can be found here: [utils] update_llc_test_checks --downstream-target-handler-path option by vporpo · Pull Request #135879 · llvm/llvm-project · GitHub

Caveats

Even though adding the option for a downstream target module decouples downstream from upstream to some extent, issues can still show up. For example, any upstream change of the target handlers interface, like changes in scrub(), get_run_handler() etc., will break downstream.

2

I don’t understand the problem. Adding a few lines of python to a script that rarely changes should be trivial in comparison to all the other changes necessary in a fork that adds a new backend. The overhead should be negligible, git pull will almost always just work, and when it doesn’t the conflict resolution should not be difficult. There’s very little churn in those scripts.

That is, option 2 is the intended way for downstreams to approach this, like extending anything else in LLVM, and I do not understand your justification for the added complexity you are proposing.

2 Likes
3

The goal is to reduce the number of patches that need to be maintained downstream, because each patch adds overhead to the pull-down process.

The change in update_llc_test_checks.py is actually pretty small, it’s one if condition (lines 131-136), one additional argument (line 85-90) and a helper function for loading the module (lines 26-38). So I would argue that it’s not adding much to the complexity of the script.