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

Mon Jun 14 11:14:30 PDT 2021

holland11 added a comment.

> 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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104149