[TOPHUB] use keys as a keyword for searching of existing statistics

[elvin-n]

In case of ARM we might not specify -device and in this case llvm will be used while even in this case we can determin proper filename with stat since keys have architecture defined. The same situatin must with with x86

[tvm-bot]

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

• No users to tag found in teams: tophub _{See #10317 for details}

_{Generated by tvm-bot}

[echuraev]

LGTM. Thanks

[ekalda]

Any chance this comment can be made clearer? :) I read it several times and I don't get it.
might have information
What information?
about it
About what?
keys container
I guess that is Target.keys? Maybe make that more explicit
in other case
What case? When keys container is empty or when -device is not defined?
irrelative stat
What is irrelative stat?

Also, is it one or multiple sentences? Capital letters and punctuation would be helpful :)

[elvin-n]

:) tried to include logic of matching statistics without detailed description in this place.

1. the only mandatory field in target is kind. it can be llvm/opencl/cuda/...
2. it might be device specified, but not required. It might be "arm_cpu", or "adreno" for example
3. it might be model specified, but not required. It might be "riscv32-unknown-linux-gnu" or "kirin970" for example
4. the keys field is not specified in the Target string but is filled after analysis of the Target automatically by devices. It is done for hierarchical search in the topi implementations and do fallback to lower/parents layers. As a result it might be ["arm_cpu", "cpu"]

Before this change, dedicated tophub file was searched only by device and second by kind
As you remember, device is not mandatory in the Target. At the same time TVM understands that target belongs to arm_cpu, it treat it as arm_cpu, but before my change, the target like llvm -model=snapdragon835 -mtriple=arm64-linux-android -mattr=+neon was matched only to llvm stat file. I cannot even imagine on which device llvm stat was collected, but definitely not on ARM. I.e. statistics was irrelevant.

Adding keys to the priority before kind we increase the probability of searching proper statistics.

Do I need to comment line by line your answer?

[ekalda]

Thanks for the detailed explanation :) No need to respond line by line, I understand what it is doing and I think it is a really good change. However, would you mind updating that comment in your PR? Otherwise the next person reading it will be confused again. Reading that comment, I think it has the logic inverted, I suppose you meant "For cases when we do NOT have explicitly defined -device in the target"? In my opinion something along the lines "If we don't have explicitly defined -device in the target, check target.keys for device." would be enough. Or even better, explicitly list the order which we use to determine the device:

1. target.device
2. target.keys
3. target.kind

In general, it is indeed pretty worrying that we can silently apply completely irrelevant parameters from the tophub. I suppose in the ideal world (out of scope for this PR) we'd have something like "Using tuning parameters determined by tuning for < specific core or architecture >" printed to the console during compilation, at least for tvmc, to warn the user about the possibly bad choice of params.

[elvin-n]

ok, will modify comment

[elvin-n]

As for applying of the statistics, I am not even sure that taking statistics from another model of the same device is a good idea. Fortunately it more or less works for ARM CPU currently but if we take intel graphics, the previous models of Iris had different limitation of group worksize. Statistics which collected on newer graphics cannot be applied on previous one. I.e. we risk not to have bad performance but have just not working kernels.

I.e. I agree that we need to do something with tophub and search algorithm, but do not have idea how we need to do this.

And probably it is not so important assuming that Metaschedule should be the tuner #1. On the other hand MS is not a solution for default compilation without tuning.

[ekalda]

Yeah I agree that applying same tuning statistics across different implementations of the same architecture is quite sketchy, especially if it happens in this non-transparent way it currently does. I'm also not sure how often the database gets updated with new stats. I haven't seen compilation crashing because of unsuitable tuning records from tophub myself, but if it happens, that's really bad, since it makes tuning compulsory for being able to compile a network with TVM. Also, sounds like a really difficult thing to debug. But I suppose as long as AutoTVM is the default tuner (as it is for tvmc) and therefore the one new potential users will use to evaluate TVM, it should be kept in a state where its behaviour is representative of TVM's capabilities. In my opinion it would be good if we didn't have 3 tuners to maintain in parallel, but I suppose that's where the community is at the moment.

[ekalda]

Thanks for the detailed explanation :) No need to respond line by line, I understand what it is doing and I think it is a really good change. However, would you mind updating that comment in your PR? Otherwise the next person reading it will be confused again. Reading that comment, I think it has the logic inverted, I suppose you meant "For cases when we do NOT have explicitly defined -device in the target"? In my opinion something along the lines "If we don't have explicitly defined -device in the target, check target.keys for device." would be enough. Or even better, explicitly list the order which we use to determine the device:

1. target.device
2. target.keys
3. target.kind

In general, it is indeed pretty worrying that we can silently apply completely irrelevant parameters from the tophub. I suppose in the ideal world (out of scope for this PR) we'd have something like "Using tuning parameters determined by tuning for < specific core or architecture >" printed to the console during compilation, at least for tvmc, to warn the user about the possibly bad choice of params.

[ekalda]

Looks great, thanks @elvin-n for updating the comment! :)