This is an archive of the discontinued LLVM Phabricator instance.

Check Sched Class tables at generation time - 2
Changes PlannedPublic

Authored by RKSimon on Jun 15 2018, 9:07 AM.

Details

Summary

It's a new version of D47637: an attempt to validate sched models at generation time. If you start TableGen with this patch you'll see something like here:

warning:SchedRW machine model for inst 'ZIP1v8i8' ( WriteV ) should be removed because it's overriden 6 times for the following models:

FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v16i8' ( WriteV ) should be removed because it's overriden 7 times for the following models:

CortexA57Model FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v2i32' ( WriteV ) should be removed because it's overriden 6 times for the following models:

FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v2i64' ( WriteV ) should be removed because it's overriden 7 times for the following models:

CortexA57Model FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v4i16' ( WriteV ) should be removed because it's overriden 6 times for the following models:

FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v4i32' ( WriteV ) should be removed because it's overriden 7 times for the following models:

CortexA57Model FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v8i16' ( WriteV ) should be removed because it's overriden 7 times for the following models:

CortexA57Model FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'ZIP2v8i8' ( WriteV ) should be removed because it's overriden 6 times for the following models:

FalkorModel KryoModel ExynosM1Model ExynosM3Model ThunderX2T99Model ThunderX2T99Model

warning:SchedRW machine model for inst 'BSWAP32r' ( WriteALU ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BSWAP64r' ( WriteALU ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BTC16mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BTC16ri8' ( WriteALU ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BTC16rr' ( WriteALU ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BTC32mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

warning:SchedRW machine model for inst 'BTC32ri8' ( WriteALU ) should be removed because it's overriden 7 times for the following models:

AtomModel SandyBridgeModel HaswellModel BroadwellModel Znver1Model SkylakeClientModel SkylakeServerModel

I hope this diagnostic could be really useful for devs.

It's not a final patch. It should be extended with other diagnostics. But I'd like to show the idea here.
JFYI: if you start the standart build process with this patch applied you'll see the new diagnostic because TableGen will be changed and used.

Diff Detail

Event Timeline

avt77 created this revision.Jun 15 2018, 9:07 AM
courbet added inline comments.Jun 18 2018, 12:22 AM
utils/TableGen/CodeGenSchedule.cpp
744

it would be interesting to say "N times out of M" to put this in perspective.

753

IMO this is actually the more useful check out of the two. I suspect that there are a lot of instances of this at least in Intel, and this is where we can remove a lot of the duplicated information.

754

I think we should always do it because it's already a win if we can remove one instruction for the list/regexp of instructions

avt77 added a comment.Jul 3 2018, 4:12 AM

Hi! I was out for 2 weeks that's why I did not do anything here. Is it still interesting for you? I'm going to publish an update asap.
But the question is: how to get latnency/uop from CodeGenSchedClass ?

avt77 updated this revision to Diff 153911.Jul 3 2018, 7:25 AM

Now it's almost completed but should deal with deafult Latency value: hope to implement it soon. (The current version does not produce yet warning about identical uOps & Latency during compiler build: maybe it's OK?)

@avt77 Next step is probably to start briefly collating the instructions causing the warnings and investigate how best to fix them (list them here, raise bugs etc.). The aim would be to see if we can remove ALL these warnings before we consider committing this patch - its probably time to add the maintainers for each target that has warnings so they can be investigated?

avt77 added a comment.Jul 9 2018, 12:14 AM

@avt77 Next step is probably to start briefly collating the instructions causing the warnings and investigate how best to fix them (list them here, raise bugs etc.). The aim would be to see if we can remove ALL these warnings before we consider committing this patch - its probably time to add the maintainers for each target that has warnings so they can be investigated?

Sorry, I missed this message. Are you sure it's better to commit after removing the warnings? I can do it for x86 models but what about others? And how we could show other maintainers the issues w/o committing of this patch?

avt77 added a comment.Jul 9 2018, 1:33 AM

In fact we have the warnings for 2 Targets only:

N: Tim Northover
E: t.p.northover@gmail.com
D: AArch64 backend, misc ARM backend

and

N: Craig Topper
E: craig.topper@gmail.com
E: craig.topper@intel.com
D: X86 Backend

I'm going to update the patch for X86 and then I'll invite the maintainers. And I did not use the default latency yet (see the line 770).

avt77 added a comment.Jul 9 2018, 3:56 AM

At the moment we have the following warns for X86:

warning: SchedRW machine model for inst 'BTC16mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC16ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC16rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC32mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC32ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC32rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC64mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC64ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTC64rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR16mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR16ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR16rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR32mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR32ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR32rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR64mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR64ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR64rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS16mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS16ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS16rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS32mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS32ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS32rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS64mi8' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS64ri8' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS64rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG16rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG32rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG64rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG8rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CVTTSS2SI64rm' ( WriteCvtSS2ILd ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CVTTSS2SI64rm_Int' ( WriteCvtSS2ILd ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FDECSTP' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FINCSTP' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FLDCW16m' ( WriteLoad ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FNINIT' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FNSTCW16m' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F16m' ( WriteLoad ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F32m' ( WriteLoad ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F64m' ( WriteLoad ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16m' ( WriteIMulLd ReadDefault ReadDefault ReadDefault ReadDefault ReadDefault ReadAfterLd ReadAfterLd ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16rri' ( WriteIMul ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16rri8' ( WriteIMul ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL32r' ( WriteIMul ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP16m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP32m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP64m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_F16m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_F32m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP16m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP32m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP64m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JCXZ' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JECXZ' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JRCXZ' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LD_F80m' ( WriteLoad ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LEAVE' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LEAVE64' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOP' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOPE' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOPNE' ( WriteJump ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'MUL16m' ( WriteIMulLd ReadDefault ReadDefault ReadDefault ReadDefault ReadDefault ReadAfterLd ReadAfterLd ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'MUL32r' ( WriteIMul ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PAUSE' ( WriteNop ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PCMPGTQrr' ( WriteVecALUX ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'POP16rmm' ( WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSH16rmm' ( WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSH32rmm' ( WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSHF16' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'RETQ' ( WriteJumpLd ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR16m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR16mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR32m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR32mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR64m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR64mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR8m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR8mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASB' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASL' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASQ' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASW' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL16m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL16mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL32m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL32mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL64m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL64mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL8m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL8mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD16mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD16mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD16rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD16rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD32mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD32mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD32rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD32rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD64mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD64mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD64rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHLD64rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR16m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR16mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR32m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR32mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR64m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR64mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR8m1' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR8mi' ( WriteShiftLd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD16mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD16mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD16rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD16rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD32mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD32mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD32rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD32rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD64mrCL' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD64mri8' ( WriteShiftDoubleLd WriteRMW ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD64rrCL' ( WriteShiftDouble ) should be removed because it's overriden 8 times out of 12 models:
warning: SchedRW machine model for inst 'SHRD64rri8' ( WriteShiftDouble ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSB' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSL' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSQ' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSW' ( WriteMicrocoded ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ST_FP80m' ( WriteStore ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'UCOM_FPr' ( WriteFCom ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'UCOM_Fr' ( WriteFCom ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'VPCMPGTQrr' ( WriteVecALUX ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD16rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD32rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD64rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD8rm' ( WriteALULd WriteRMW ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG16ar' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG16rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG32ar' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG32rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG64ar' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG64rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XCHG8rr' ( WriteALU ) should be removed because it's overriden 7 times out of 12 models:

I removed the warns for BSWAP instr and going to publish the fix soon.

avt77 updated this revision to Diff 154580.Jul 9 2018, 5:28 AM
avt77 added reviewers: craig.topper, t.p.northover.

I fixed warns for BSWAP instrs (see changes in X86*.td files). If this approach is OK I'll fix other instrs.

Please can you pul out the BSWAP change into its own patch for review?

lib/Target/X86/X86InstrInfo.td
1353 ↗(On Diff #154580)

Just use WriteBSWAP - you have 16, 32 and 64 size instructions here

RKSimon added a subscriber: lebedev.ri.

@lebedev.ri What do you reckon we should do to fix the WriteShiftDouble warnings?

@lebedev.ri What do you reckon we should do to fix the WriteShiftDouble warnings?

Yay, more warnings is always nice, thanks for working on this!

For bdver2, not counting the variants with memory operands, since llvm-exegesis can't measure those yet,
the WriteShiftDouble as i have added it, did fit perfectly, at least at the moment,
so i would opt into not dropping WriteShiftDouble completely.

Other than that, i'm afraid i do not yet understand the question/problem...

avt77 added a comment.Jul 11 2018, 7:50 AM

Please can you pul out the BSWAP change into its own patch for review?

OK, I'll do it for BSWAP and (separately) for BT(C|S...): I'm working on it just now.

lib/Target/X86/X86InstrInfo.td
1353 ↗(On Diff #154580)

You're right but we have different latency/throughput for different sizes that's why I supposed to use different WriteBSWAP. At the moment I simply kept the instrs redefinition for 16/64 as it is.
For BT* instrs we have the same numbers for all sizes - I'm working on it.

avt77 updated this revision to Diff 155142.Jul 12 2018, 3:08 AM

Now the patch keeps only infrastructure changes to produce new TableGen warns about sched models.

Nice! Some nits.

utils/TableGen/CodeGenSchedule.cpp
41–44

Based on the // FIXME: TableGen is failed iff EXPENSIVE_CHECKS defined i think this is inverted?

// FIXME: the default value should be true
static cl::opt<bool> OptCheckSchedClasses(
    "check-sched-class-table", cl::init(false), cl::Hidden,
    cl::desc("Check sched class table on different types of inconsistencies"));
721

s/if its overridden/if its overridden too often/

743

I really don't like word removed here. It's misleading.
Based on previous comments, it should reflect the idea that it
should be *refined* into more, more fine-grained sched classes, not removed.

753

latency

764
for (unsigned I = 0, S = SC.Writes.size(); I < S; I++) {

i think

765–785

Using some tmp variables for SchedWrites[SC.Writes[I]] and SchedWrites[Writes[I]] might help.

772–785

Clearly, getting a bit too nested.
Maybe refactor into helper function?

avt77 added inline comments.Jul 16 2018, 1:53 AM
utils/TableGen/CodeGenSchedule.cpp
41–44

Why? By default we should not do this check that's why it should be false.

lebedev.ri added inline comments.Jul 16 2018, 2:00 AM
utils/TableGen/CodeGenSchedule.cpp
41–44

If it's not enabled by default in non-release builds (say, when assertions are enabled), things will regress.
But i may be missing the point.

avt77 added inline comments.Jul 16 2018, 7:23 AM
utils/TableGen/CodeGenSchedule.cpp
41–44

We tried to do it by default if EXPENSIVE_CHECKS is defined but unfortunately LLVM can't build TableGen with EXPENSIVE_CHECKS defined. As result I introduced this special switch and of course it should be off by default (e.g. it could increase the compilation time).

avt77 updated this revision to Diff 155674.Jul 16 2018, 7:31 AM

It seems I fixed all issues raised by lebedev.ri. The open question is: should I remove the second check inside checkSchedClasses? I'm sure there is a way to get default values for such SchedWrites like WriteALU, WriteFAdd, etc. but I don't know at the moment how to do it. Please, help.

(Would be great if the checkbox 'done' in notes would be getting checked, too)

utils/TableGen/CodeGenSchedule.cpp
41–44

Yeah, i filed https://bugs.llvm.org/show_bug.cgi?id=38172 yesterday, didn't find any previous bugreport.
But that didn't address my question. If OptCheckSchedClasses == true, the checking happens.
Not the other way around. So i don't understand how the current defaults, and the comments relate.

avt77 added a comment.Jul 17 2018, 2:28 AM

(Would be great if the checkbox 'done' in notes would be getting checked, too)

Sorry, but I don't understand what do you mean here?

utils/TableGen/CodeGenSchedule.cpp
41–44

Yes, If OptCheckSchedClasses == true, the checking happens. And by default we should not do it because (in theory) it could be rather expensive check. But I use cl::init(true) to be able to see warnings every launch of TableGen. In product version it should be false. Am I right? Should I put 'false' in?

What happened to the BSWAP patch? Compared to BT (D49243) it should have been pretty trivial to implement.

utils/TableGen/CodeGenSchedule.cpp
778

(style) Don't use auto here - please use the full types

avt77 added a comment.Jul 17 2018, 8:26 AM

What happened to the BSWAP patch? Compared to BT (D49243) it should have been pretty trivial to implement.

In fact we have there very similar issues but even with rr versions: we should separate implementations for different sizes 16/32/64. I'm going to complete it tomorrow. But again - most probably it will be w/o memory operands support like in D49243.

avt77 updated this revision to Diff 155909.Jul 17 2018, 10:04 AM

I replaced 'auto' with real types accordingly to Simon's request.

avt77 added a comment.Aug 9 2018, 4:48 AM

Currently we have 93 warnings for X86 CPU models:

warning: SchedRW machine model for inst 'BTC16mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models
warning: SchedRW machine model for inst 'BTC32mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models
warning: SchedRW machine model for inst 'BTC64mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR16mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR32mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTR64mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS16mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS32mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'BTS64mi8' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG16rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG32rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG64rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CMPXCHG8rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CVTTSS2SI64rm' ( WriteCvtSS2ILd ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'CVTTSS2SI64rm_Int' ( WriteCvtSS2ILd ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FDECSTP' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FINCSTP' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FLDCW16m' ( WriteLoad ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FNINIT' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'FNSTCW16m' ( WriteALU ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F16m' ( WriteLoad ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F32m' ( WriteLoad ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ILD_F64m' ( WriteLoad ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16m' ( WriteIMulLd ReadDefault ReadDefault ReadDefault ReadDefault ReadDefault ReadAfterLd ReadAfterLd ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16rri' ( WriteIMul ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL16rri8' ( WriteIMul ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IMUL32r' ( WriteIMul ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP16m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP32m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ISTT_FP64m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_F16m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_F32m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP16m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP32m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'IST_FP64m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JCXZ' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JECXZ' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'JRCXZ' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LD_F80m' ( WriteLoad ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LEAVE' ( WriteALU ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LEAVE64' ( WriteALU ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOP' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOPE' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'LOOPNE' ( WriteJump ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'MUL16m' ( WriteIMulLd ReadDefault ReadDefault ReadDefault ReadDefault ReadDefault ReadAfterLd ReadAfterLd ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'MUL32r' ( WriteIMul ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PAUSE' ( WriteNop ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PCMPGTQrr' ( WriteVecALUX ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'POP16rmm' ( WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSH16rmm' ( WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSH32rmm' ( WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'PUSHF16' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'RETQ' ( WriteJumpLd ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR16m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR16mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR32m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR32mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR64m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR64mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR8m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SAR8mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASB' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASL' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASQ' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SCASW' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL16m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL16mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL32m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL32mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL64m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL64mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL8m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHL8mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR16m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR16mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR32m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR32mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR64m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR64mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR8m1' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'SHR8mi' ( WriteShiftLd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSB' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSL' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSQ' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'STOSW' ( WriteMicrocoded ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'ST_FP80m' ( WriteStore ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'UCOM_FPr' ( WriteFCom ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'UCOM_Fr' ( WriteFCom ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'VPCMPGTQrr' ( WriteVecALUX ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD16rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD32rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD64rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:
warning: SchedRW machine model for inst 'XADD8rm' ( WriteALULd WriteRMW ) should be updated /improved because it's overriden 7 times out of 12 models:

I'm working with BT*m*, CMPXCHG* and XADD*m instrs.

An idea for further checking:

  • If an instruction I is overridden in the sched class,
  • And there is a load-folded version of that instruction,
  • And that load-folded version is not overridden

then it should be diagnosed, since it is likely an oversight.

lebedev.ri requested changes to this revision.Jul 10 2019, 3:31 PM

(removing from my review queue)

This revision now requires changes to proceed.Jul 10 2019, 3:31 PM
RKSimon commandeered this revision.Nov 13 2022, 6:11 AM
RKSimon edited reviewers, added: avt77; removed: RKSimon.

Commandeering this old patch - I have various local patches that this was based on that I'd like to get upstreamed

Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2022, 6:11 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
RKSimon planned changes to this revision.Nov 13 2022, 6:11 AM