This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix in ICE when retrieving the number of micro-ops for vlldm/vlstm
ClosedPublic

Authored by chill on Dec 10 2019, 6:24 AM.

Details

Summary

The big switch in ARMBaseInstrInfo::getNumMicroOps is missing cases for
VLLDM and VLSTM, which are currently defined with itineraries having a
dynamic count of micro-ops.

Assuming an optimistic case in which these instruction do not actually perform
loads or stores, and with the idea that Armv8-m cores are supposed to use the
new style scheduling models, this patch just sets the itinerary for those two
instructions to NoItinerary.

Diff Detail

Event Timeline

chill created this revision.Dec 10 2019, 6:24 AM

Is there some reason we can't handle these using tablegen'ed data, like other instructions?

I think that this function is only considering Itineraries? When a MISchedule isn't present? They are a bit Legacy now, and I wouldn't expect any cpu with these instructions to be using them.

It looks like in the instruction definitions, the VLSTM and VLLDM have IIC_fpLoad_m and IIC_fpStore_m. That would appear to be where the "multi-uops" is coming from. Can we just remove that? Use no itineraries instead? At the very least, like you said, the VLLDM and VLSTM are lazy so will not often not infer a cost at the point of them being ran. Maybe a NOP, if there is one? But I think None would be fine too.

chill added a comment.Dec 11 2019, 3:51 AM

Is there some reason we can't handle these using tablegen'ed data, like other instructions?

In a sense we do use tablegen, it just that tablegen says "defer that to C++ code". (cf. "dynamic uops" in ARMScheduleA8.td and ARMScheduleA9.td).

It looks like in the instruction definitions, the VLSTM and VLLDM have IIC_fpLoad_m and IIC_fpStore_m.

Yeah, I was thinking of setting these to NoItinerary. I thought I wouldn't like to disturb existing scheduling, but on a second thought,
indeed, we'd better have new-style scheduling models for armv8-m cores.

I will see about removing IIC_fpLoad, etc.

chill updated this revision to Diff 233544.Dec 12 2019, 2:37 AM
dmgreen accepted this revision.Dec 12 2019, 6:10 AM

LGTM. Cheers

This revision is now accepted and ready to land.Dec 12 2019, 6:10 AM
chill retitled this revision from [ARM] Return a number of micro-ops for vlldm/vlstm to [ARM] Fix in ICE when retrieving the number of micro-ops for vlldm/vlstm.Dec 13 2019, 10:16 AM
chill edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.