[SchedModel] Missing ReadAdvance for the source (non dest) operand of RMW instructions.

[adibiagio]

Bugzilla Link	51322
Resolution	FIXED
Resolved on	Oct 11, 2021 20:29
Version	trunk
OS	Windows NT
Blocks	#51489
CC	@topperc,@LebedevRI,@RKSimon,@phoebewang,@rotateright,@tstellar
Fixed by commit(s)	f0658c7 7a1a35a d8667f1 20eced2

Extended Description

This is similar to bug 51318. However, it doesn't only affect ADC/SBB but any RMW arithmetic opcode.

Example:

add  %eax, %eax
add  %eax, 4(%rsp)

llvm-mca -mcpu=btver2 -iterations=1 -timeline

Timeline view:
Index     0123456789

[0,0]     DeER .   .   addl     %eax, %eax
[0,1]     D=eeeeeeER   addl     %eax, (%rsp)

EAX is written in 1cy. However, the load from RSP can start immediately, and doesn't need to be delayed for 1cy.

This is the expected timeline:

Timeline view:
Index     012345678

[0,0]     DeER .  .   addl      %eax, %eax
[0,1]     DeeeeeeER   addl      %eax, 4(%rsp)

The issue is with the definition of WriteALURMW. it is caused by the absence of a ReadAdvance for the register operand.

According to llvm-mc:

        addl    %eax, 4(%rsp)                   # <MCInst #&#8203;380 ADD32mr
                                        #  <MCOperand Reg:58>
                                        #  <MCOperand Imm:1>
                                        #  <MCOperand Reg:0>
                                        #  <MCOperand Imm:4>
                                        #  <MCOperand Reg:0>
                                        #  <MCOperand Reg:22>>

The first 5 MCOperands are for the memory reference 4(%rsp).
The read at index #​5 is currently not marked as ReadAfterLd.

Same problem affects ADC/SBB, for which we use an almost identical pattern, except that WriteADCRMW is used instead of WriteALURMW.

For those instructions, we also need to mark with ReadAfterLd the implcit read of EFLAGS.

[adibiagio]

I have a fix for this (and bug 51318) which I plan to send for review today.

[adibiagio]

https://reviews.llvm.org/D107367

[LebedevRI]

Presumably this should be picked into the release branch?

[adibiagio]

Presumably this should be picked into the release branch?

CC'ing Tom.

It would be really nice if we could merge this fix into the release branch.

It fixes a common pattern in code. It could lead to better instruction schedules (so, possibly better perf); the improvement would not be limited to just mca.

P.s.: Our numbers for those patterns are now more in line with what IACA generates. The "odd" mca throughput reported in https://uica.uops.info/ (on the default code snippet) is fixed by this patch.

[tstellar]

The fix does not apply cleanly, could someone backport this and push a branch to their local github fork?

[adibiagio]

Hi Tom,

This commit depends on another commit which added and modified some tests.

Tests for this fix were added by commit f0658c7

So I think it should work if you:

git cherry-pick -x f0658c7

and then

git cherry-pick -x 7a1a35a

[tstellar]

Merged: 20eced2

[tstellar]

mentioned in issue #51489