Page MenuHomePhabricator

[SchedModel] Remove instregex entries that don't match any instructions (WIP)
ClosedPublic

Authored by RKSimon on Mar 20 2018, 9:15 AM.

Details

Summary

This patch throws a fatal error if an instregex entry doesn't actually match any instructions. This is part of the work to reduce the compile time impact of increased instregex usage (PR35955), although the x86 models seem to be relatively clean.

I've included hacks for all the cases I encountered, I don't know if I've fixed them correctly though (e.g. AArch64SchedThunderX2T99 has a lot of duplicate patterns for its ALU instructions).

I've also included a disabled error for cases where the instregex only matches one instruction - which IMO should probably be replaced with a instrs entry - this might be worth just a warning or we remove it from the patch completely (but it'd definitely improve compile time).

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Mar 20 2018, 9:15 AM

The changes to CodeGenSchedule.cpp looks good to me. I will let other people review the AArch64/ARM scheduling model changes.
P.s.: I also like the idea of having a warning (at least in debug mode) for when there is only one matching instruction.

craig.topper added inline comments.Mar 20 2018, 11:04 AM
utils/TableGen/CodeGenSchedule.cpp
109 ↗(On Diff #139141)

Do we need two loops here? Why are we building a list of Regexs and the iterating over it?

RKSimon added inline comments.
utils/TableGen/CodeGenSchedule.cpp
109 ↗(On Diff #139141)

I haven't looked into the history of this code, although I think @bkramer refactored some of it not so long ago.

I agree that the loops should probably be merged, I'm happy to cleanup this up as long as it doesn't get in the way of people investigating the issues in the models.

I'd like to see the loops merged to get rid of the nested std::pairs. But yeah I don't want to get in the way of fixing all the targets.

The X86 changes look to me. That FMA was one in Zen is interesting. Does the the ZnWriteResFpuPair for FMA have the right information that prevented that from affecting tests?

I'd like to see the loops merged to get rid of the nested std::pairs. But yeah I don't want to get in the way of fixing all the targets.

The X86 changes look to me. That FMA was one in Zen is interesting. Does the the ZnWriteResFpuPair for FMA have the right information that prevented that from affecting tests?

Yes, the override is probably redundant - see PR35548

fhahn added inline comments.
lib/Target/AArch64/AArch64SchedThunderX2T99.td
426 ↗(On Diff #139217)

Those changes should be fine (and similar changes below), as they just remove invalid patterns.

1264 ↗(On Diff #139217)

MOV (AdvSIMD) is an alias for INS. MVN is an alias of NOT. So I think to keep the model unchanged, we should use INS and NOT here.

@joelkevinjones @steleman could you have a quick look to confirm?

Also, the comment just above needs updating.

1516 ↗(On Diff #139217)

Looks OK, MOVIv should match vector versions of MOVI.

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

fhahn added a comment.Mar 21 2018, 3:21 AM

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

Yep, I can commit the AArch64 changes. I'll wait till tomorrow, in case Joel or Stefan have any concerns.

Thanks for this. If I am getting this right, just like CompleteModel ensures no instruction has a missing schedule, this patch will ensure no instruction has multiple schedule (instregex) assigned?

lib/Target/ARM/ARMScheduleR52.td
228 ↗(On Diff #139217)

Not sure why you removed 't2MOV_ga_pcrel_ldr' ? Is it repeated somewhere else ?

287 ↗(On Diff #139217)

t2MLALBB missing?

utils/TableGen/CodeGenSchedule.cpp
152 ↗(On Diff #139217)

Just wondering why having 'just one match' would be a problem?

Thanks for this. If I am getting this right, just like CompleteModel ensures no instruction has a missing schedule, this patch will ensure no instruction has multiple schedule (instregex) assigned?

Not quite (thats another check that I'll be adding soon....) - this patch just errors out if the instregex is not used at all - from the cases I've seen its usually a mixture of copy+paste (not all instructions have all the same variations) and typos in the regex pattern.

This patch makes sure each regular expression covers at least one instruction. We already checked that each InstRW line matched at least one instruction. But if there were multiple regular expressions listed, we didn't check it.

Making sure each instruction is only covered by one instregex or instrs match is already being done. And a hole in it was fixed recently with r327808. Though I had to disable the improved check on some models. Look for FullInstRWOverlapCheck = 0 in the SchedMachineModel in the td files to see if I disabled it for any schedulers you care about. I need to collect the effected schedulers and start getting people to fix it.

lib/Target/ARM/ARMScheduleR52.td
228 ↗(On Diff #139217)

According to ARMGenInstrInfo.inc, there is no t2MOV_ga_pcrel_ldr

287 ↗(On Diff #139217)

t2MLALBB doesn't exist in ARMGenInstrInfo.inc

utils/TableGen/CodeGenSchedule.cpp
152 ↗(On Diff #139217)

Having just one match means you should probably be using "(instrs" instead of "(instregex". The regular expression has to get tried on every instruction name that is defined. If there's ever only going to be one match that just wastes tablegen build time.

RKSimon added inline comments.Mar 21 2018, 9:01 AM
lib/Target/ARM/ARMScheduleR52.td
228 ↗(On Diff #139217)

Searching through ARMGenInstrInfo.inc I could see MOV_ga_pcrel_ldr but not t2MOV_ga_pcrel_ldr - that was what caused the error.

287 ↗(On Diff #139217)

Yup - doesn't exist in ARMGenInstrInfo.inc

utils/TableGen/CodeGenSchedule.cpp
152 ↗(On Diff #139217)

Every regex pattern has to match against every instruction string - its quite costly - we're looking at ways to speed this up (PR35955).

I'm not sure about enabling this - that's (a) why its currently #if'd out and (b) why I might reduce this to a warning if we keep it at all.

This patch makes sure each regular expression covers at least one instruction. We already checked that each InstRW line matched at least one instruction. But if there were multiple regular expressions listed, we didn't check it.

Making sure each instruction is only covered by one instregex or instrs match is already being done. And a hole in it was fixed recently with r327808. Though I had to disable the improved check on some models. Look for FullInstRWOverlapCheck = 0 in the SchedMachineModel in the td files to see if I disabled it for any schedulers you care about. I need to collect the effected schedulers and start getting people to fix it.

OK, I missed that. Please add R52 to your list. FullInstRWOverlapCheck idea is great. When there is overlap, would it be possible to check if the Defs are identical (less serious case) as opposed to two conflicting sched for same instruction? Sorry, maybe i should ask in a separate review (please point me to it).

GGanesh added inline comments.Mar 21 2018, 9:34 PM
lib/Target/X86/X86ScheduleZnver1.td
919 ↗(On Diff #139217)

Looks okay to me.

fhahn added a comment.Mar 23 2018, 4:05 AM

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

Yep, I can commit the AArch64 changes. I'll wait till tomorrow, in case Joel or Stefan have any concerns.

Done, rL328303

RKSimon updated this revision to Diff 139584.Mar 23 2018, 6:37 AM

Rebased after recent scheduler fixes - just the ARM cases still need to be reviewed/committed now.

I've removed the single match code - we can revisit this in the future but for now the number of warnings is just too much for a buildtime performance issues. I'll try to work on at least fixing the x86 cases.

This revision is now accepted and ready to land.Mar 25 2018, 11:36 AM
This revision was automatically updated to reflect the committed changes.

Looks good to me!