Page MenuHomePhabricator

[AArch64][SVE] Asm: Set SVE as unsupported feature for existing scheduler models

Authored by sdesmalen on Oct 19 2017, 5:04 AM.



Patch [4/5] in a series to add assembler/disassembler support for AArch64 SVE unpredicated ADD/SUB instructions.

We add SVE as unsupported feature for CPUs that don't have SVE to prevent errors from scheduler models saying it lacks information for these instructions.

Diff Detail


Event Timeline

sdesmalen created this revision.Oct 19 2017, 5:04 AM
javed.absar added inline comments.Oct 19 2017, 9:23 AM
21 ↗(On Diff #119571)

nitpick. The blank line here and in other .td not needed i think.

fhahn added a subscriber: fhahn.Oct 19 2017, 9:35 AM
fhahn added a comment.Oct 19 2017, 9:38 AM

I think it would be good to explain why this change is necessary in the review description. Why do we need to add UnsupportedFeatures to the existing scheduling models for SVE?

sdesmalen marked an inline comment as done.Oct 20 2017, 7:38 AM

@fhahn The reason for adding it as unsupported feature (besides the fact that these CPUs don't have SVE), is that we otherwise would get errors like:

'ThunderXT8XModel' lacks information for <name of SVE instruction here>.

Thanks for the suggestion, I'll add it to the description of the patch.

sdesmalen edited the summary of this revision. (Show Details)Oct 20 2017, 8:43 AM
rengolin edited edge metadata.Oct 24 2017, 10:27 AM

Simple and straightforward. Looks good, but will approve once all patches are good.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

fhahn added a comment.Oct 31 2017, 6:45 AM

The reason we need this are over-eager regular expression in scheduling models, e.g. some models match TRN1 instructions for all types and the scheduler then fails to compile, because scheduling info is missing for SVE instructions. I think ideally those regexs should be more specific, but using UnsupportedFeatures seems like a more practical way to handle this problem and is also used by SystemZ, PPC and MIPS targets.

Yes, this change makes sense to me.

rengolin accepted this revision.Oct 31 2017, 7:16 AM
This revision is now accepted and ready to land.Oct 31 2017, 7:16 AM
This revision was automatically updated to reflect the committed changes.