I really like this JSON format. Thanks a lot!

The json output is better now.
However there are things about the implementation that must be fixed/changed (see my comments below).

Thanks for checking.

In D92026#2413946, @evgeny777 wrote:

When you say that " Subtarget files are unchanged by the patch", does it mean that you have checked all the targets or just ARM?

I checked ARM, AArch64 and X86. Does any other have scheduling enabled?

I've only skimmed through the code, but it looks like a nice improvement.

LGTM

A couple of minor nits. Otherwise the llvm-mca change looks good.
Thanks.

Thanks for the updated patch.

LGTM for llvm-mca.

Much better. Thanks a lot :)

The new predicate looks good. I only have a couple of nits (see below).

In D90024#2350657, @dmgreen wrote:

I'm happy with that explanation, and would be happy with this patch going ahead. The mir test above seems to show it failing before and working now.

So, it doesn't look like the lowering is doing anything odd with the operand sequence. It should be fine.
At this point, the only explanation is that the original predicate was never really executed for that write variant. So it was never tested. I don't see other alternatives honestly.

In D90024#2350536, @dmgreen wrote:

(paranoia level).
Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...

I was not aware that was a thing that could happen!

I'm unsure if anything would ever generate an ldrbt with negative postinc reg and shift from codegen, so it might be difficult to test. And may never have been tested in the past.

In D90024#2350392, @evgeny777 wrote:

Should it always have been checking operand 4 then? I think that makes sense

IMO, it was incorrectly checking for operand 3. I've got an assertion when switched to MC pred

In D90024#2350389, @dmgreen wrote:

Should it always have been checking operand 4 then? I think that makes sense

(More of a commit message might make it more obvious why changes are being made ;) )

Nice!
The predicates look good to me.

The new predicate looks good to me.

Could you please add more context to the diff? I don't remember how IsLdstsoScaledPredX2 was defined.

LGTM.

Nice patch!
The new predicate looks good.

In D89876#2344978, @evgeny777 wrote:

Addressed

Hi Eugene,

I have been thinking a bit more about this patch.
D89553 is surprisingly less complicated than I have originally thought it was. It definitely touches more files. However the majority of the changes are very mechanical (mostly boilerplate required for the new MCSchedPredicate to work). It is also a non-controversial change, since it doesn't modify the layout of MCInst.
Bonus point: by changing the signature of resolveVariantSchedClass() we potentially enable future developments in the area of MCSchedPredicate (and mca). Basically that change would make it possible for users to declare new predicates on MCinstrDesc (which is definitely not a bad thing!).

The patch is mostly a mechanical change (i.e. a lot of boilerplate to introduce a new scheduling predicate).

For the record, this is how your patch would look like if you decided to add a new predicate.

Hey Wolfgang,
are you still looking at this patch? Do you have any updates?

Looks good to me.

In D86644#2240251, @wolfgangp wrote:

In D86644#2239673, @lebedev.ri wrote:

Can it simply be autogenerated, without omitting details?

It would have to be auto-generated based on the readable output, then, using some sort of (python) script that creates the structure independently, or did you have something else in mind?

Thanks Wolfgang for working on this.

Looks good to me module a few nits (see below).

LGTM

In D86177#2227126, @wolfgangp wrote:

In D86177#2225748, @andreadb wrote:

Nice!

Thank you Andrea!

I agree with Roman in that we should not guarantee stability of data and/or structure.
Also, changes to the output structure should always be advertised (for example, by adding a line in the release notes), so that people are always aware of it.

I must confess I am not very informed about the issues regarding stability. Doesn't data stability depend on the scheduling model? I don't see how llvm-mca could guarantee it if that changes.

Thanks Wolfgang! I always wanted to add a structured output to llvm-mca.
Thanks for working on this.

Cheers. Now I have a better understanding of why you wanted to check latencies.

In D85165#2193969, @craig.topper wrote:

@andreadb @RKSimon or @efriedma do any of you have suggestions for simple scheduler tests for this? I was hoping I could use -print-schedule like we used to but that no longer exists.

Addressed review comments.

Patch updated. This time with context.

Addressed review comments.

In D79351#2018657, @mattd wrote:

I'm totally cool with this change; however, it's been a while since I've taken a look at this part of MCA. I'll let other's chime in as well, but +1 from me.

Patch updated. This time with full context.

LGTM

In D76580#1939897, @MaskRay wrote:

Rebase after MCInstPrinter::PrintBranchImmAsAddress is added to the parent patch.

llvm-mc -filetype=asm and llvm-mca are unaffected now.

In D75214#1911336, @lebedev.ri wrote:

In D75214#1895126, @andreadb wrote:

we still need to keep into account that processors may implement loop caches.

I agree this may be useful, but i currently don't believe that to be a blocker here.

Looks good to me.

Hi Roman,

In D74155#1862443, @lebedev.ri wrote:

Okay. Then this needs tests.
There's a lot of bugs referenced in https://bugs.llvm.org/show_bug.cgi?id=44539,
at least one of those should be affected, right?

Forgot to say that this should also be backported to the release branch.

LGTM (modulo Roman comment). It fixes the regression in libgav1. So I am happy.

LGTM too.

In D74000#1859033, @andreadb wrote:

I've added avx512 gather instructions to llvm-mca resource tests. I wanted to pre-commit them, but since some of them have 0 uops in the existing data, llvm-mca gave an error.

What I can do in the short term is to remove the check for MayLoad and MayStore in mca (I have a patch ready for it). That check is too conservative, since we were already test if an instruction consumes processor resources in general. That is enough to fix the issue that you saw with zero uOP gathers.

I've added avx512 gather instructions to llvm-mca resource tests. I wanted to pre-commit them, but since some of them have 0 uops in the existing data, llvm-mca gave an error.

In D73844#1854616, @RKSimon wrote:

In D73844#1854361, @andreadb wrote:

Here is the idea: why don't we just add new writes for gather instructions (ideally a new write per each type of gather)?. I think that we shall consider this alternative (maybe in a follow up patch? I don't particularly mind..). Not sure if other people agree with this.

Adding gather (and broadcast) schedule classes is on my backlog - as long as the InstRW entries are correct, I'll replace them when I get aroung to doing it. So, staying with InstRW for this patch is fine by me.

The change looks pretty mechanical, and I trust that the new numbers are correct.

LGTM

Posted the output from llvm-exegesis for all the affected instructions.

In D67950#1705613, @andreadb wrote:

I am not convinced that this patch is correct. Isn’t the problem that your model was wrongly marked as complete?

I am not convinced that this patch is correct. Isn’t the problem that your model was wrongly marked as complete?

LGTM

Thanks Roman.

Thanks Roman,

LGTM

Looks good to me.
Thanks for the changes in FixupBWInsts. Personally I was already happy with the previous version patch. But this one is looks even better.