[PATCH] D104149: [MCA] Adding the CustomBehaviour class to llvm-mca

Mon Jun 14 12:15:00 PDT 2021

andreadb added a comment.

In D104149#2817469 <https://reviews.llvm.org/D104149#2817469>, @holland11 wrote:

>> I don't like the idea of using your new framework to workaround problems/bugs that are not in llvm-mca.
>>
>> If you think that there is a genuine bug in their instruction definitions, then please raise a bug for it.
>>
>> We should never "fix" non-mca issues within mca. This is true in general, and it doesn't only apply to mca. If there is a bug in someone else's code, we create a bug report for it, and optionally we send a patch for it. We don't pretend that the issue doesn't exist, and (worse) hack some workarounds for those bugs in our code.
>
> Yeah this makes sense. I suppose I'm just trying to find different use cases where this patch could be useful in the future. That way, I can make sure that the framework is already setup in a way that facilitates that use case rather than myself or someone else needing to tweak it to do what they want. I think there are legitimate cases where we'd want to use this pattern (removing a Def from an instruction) without wanting to touch the tablegen. But this probably isn't one of those cases so I should just be using it to make sure the design is reasonable rather than trying to submit the example upstream.
>
> In the event that I do completely remove the AMDGPU example from the patch (and then submit bug reports for all the issues I found), should I still keep an empty AMDGPUCustomBehaviour class within the patch so there is at least one example implementation? That way, if anyone wants to implement their own CB for their target, they have an example to look at for setting it up (mainly with respect to the directory structure and cmake files).

As far as I understand, your CustomBehaviour plus the RetireOOO change does improve the AMDGPU simulation.
@foad, what do you think about it?

Basically, if the AMDGPU change is a good improvement, then I still think that it makes sense to keep that target specific code + test.

About future developments:
I'd be happy to review a patch that checks for the presence of invalid writes. I have been thinking about it, and I believe that there is no harm in accepting a patch like that one. I also agree that there may be legitimate use cases for removing register definitions. So, I am fine with it. The only thing is: please, send it as a follow-up patch :)

The more I think about it, the more I believe that your framework is powerful enough to unblock a number of interesting use cases. For example, we could use it to annotate instructions based on external data generated - for example - by a profile. That way, we could refine latencies for loads; branch probabilities (if we decide to improve the simulator and enable different traces of executions); etc. These are just ideas...

Back to the patch: let see what @foad says about the new tests. If he is happy with the new numbers, then I have no further objections, and the patch can go in with the existing tests / changes to AMDGPU. If worse comes to worse, I am willing to accept your patch anyway, because I think this framework is very useful in general.

-Andrea

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104149