[PATCH] D117451: [MCA] Proposing the InstrPostProcess:modifyInstrDesc() method.

Sat Mar 12 03:48:35 PST 2022

andreadb added a comment.

In D117451#3376588 <https://reviews.llvm.org/D117451#3376588>, @holland11 wrote:

> I really like your ideas and I played around with a few different ways to implement variations of them. To do it properly would likely be quite disruptive which isn't necessarily a bad thing, but it would require thorough testing to make sure there were no bugs or flaws in my logic. The internal testing that already exists would be very useful in making sure that I didn't break mca's existing behaviour, but those tests wouldn't help me verify that CustomBehaviour and InstrPostProcess would still be modifying things in the expected and correct way. To do that, I would need to write a bunch of my own tests where I implement modifications for a target with CB & IPP and then ensure that the new mca simulations behave as intended and expected.
>
> Therefore, to be able to implement the changes that I'd like to in the way that I'd like to, I'd need to be able to test them with an instruction set that I am intimately familiar with. That instruction set would be the one that I worked with in the summer and will be working with again starting next month. Over the summer, I worked as an intern on Apple's GPU Compiler team and am starting back full-time with them in April. Since I am not currently employed by them, I do not have access to the instruction set and so I wouldn't feel confident enough in my ability to implement all of the desired changes without introducing some serious flaws or bugs.
>
> I do still think that making the API more restricted and self-explanatory is a really good idea. I also think that giving developers a way to use the API to disable specific uses / defs for any given instruction could be really useful. These things should be done properly though and my current familiarity of open source instruction sets isn't strong enough for me to feel confident in verifying my work.
>
> The main reason that I initially started working on all of this was because I wanted to make it easy to have the RetireOOO flag be set globally without having to modify tablegen. Your suggestion of copying the instruction flags from the InstrDesc object into the Instruction object would actually accomplish that (and do it in a minimally disruptive way) so I've decided to make that change. I hope and plan to come back to this in the not-too-distance future to make the restrictive API and disabling uses / defs changes, but for now, this is the change that I am most comfortable with and that accomplishes my most basic goal with this patch.
>
> This updated patch is quite a bit different from the initial one posted with this diff so I think it's probably best to make a new diff for it and close this one.
>
> Edit: Here is the new diff https://reviews.llvm.org/D121508

Thanks Patrick. I understand your situation.

Feel free to come back to this patch whenever you feel more comfortable about it. There is no need to rush :-) .

Meanwhile, I had a look at the other patch, and it looks good to me (modulo a few nits).

As always, thanks for sending patches to mca!

I really appreciate your contributions.

-Andrea

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117451