Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TableGen] Add PreferSmallerInstructions for Targets. #83587

Merged

Conversation

AlfieRichardsArm
Copy link
Contributor

This option means that in assembly matching instructions with smaller encodings will be preferred.

This will be used for the ARM instruction set where this is the correct behavior after some other refactoring.

@AlfieRichardsArm
Copy link
Contributor Author

@kosarev This is a PR for the addition of the PreferSmallInstructions option

Copy link

github-actions bot commented Mar 1, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@kosarev kosarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM modulo the typo. But please don't submit until this is actually needed.

llvm/include/llvm/Target/Target.td Outdated Show resolved Hide resolved
@AlfieRichardsArm
Copy link
Contributor Author

@s-barannikov Regarding

I think this check should be tie-breaking rather than defining, and so it should be moved below, after we've compared the number of operands and their classes.
Also, does this check supersede the AVX/AVX512 resolution below?

It is my understanding this needs to be here for ARM. The architecture reference manual states:

If neither .W nor .N is specified, the assembler can select either a 16-bit or 32-bit encoding. If both encoding
lengths are available, it must select a 16-bit encoding.

There are cases (such as thumb 1 instructions with a CCOut operand whereas thumb 2 doesnt) where the smaller instruction has more operands, so choosing that first would make this non-compliant.

@AlfieRichardsArm
Copy link
Contributor Author

AlfieRichardsArm commented Mar 5, 2024

@s-barannikov regarding

Should it be a member of AsmParser instead?

I have just pushed a commit that moves it.

llvm/utils/TableGen/AsmMatcherEmitter.cpp Outdated Show resolved Hide resolved
@@ -346,6 +342,10 @@ Record *CodeGenTarget::getAsmParser() const {
return LI[AsmParserNum];
}

bool CodeGenTarget::getPreferSmallerInstructions() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to AsmMatcherEmitter.cpp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@AlfieRichardsArm AlfieRichardsArm force-pushed the add-prefer-smaller-instructions branch 2 times, most recently from cd1b3e3 to 7c3b6e1 Compare March 7, 2024 15:09
@AlfieRichardsArm AlfieRichardsArm changed the title [TabeGen] Add PreferSmallerInstructions for Targets. [TabelGen] Add PreferSmallerInstructions for Targets. Mar 7, 2024
@AlfieRichardsArm AlfieRichardsArm changed the title [TabelGen] Add PreferSmallerInstructions for Targets. [TableGen] Add PreferSmallerInstructions for Targets. Mar 7, 2024
@AlfieRichardsArm AlfieRichardsArm force-pushed the add-prefer-smaller-instructions branch 2 times, most recently from 51e398a to 992a749 Compare March 13, 2024 16:49
This option means that in assembly matching instructions with smaller
encodings will be preferred.

This will be used for the ARM instruciton set where this is the correct
behaviour after some other refactoring.
@AlfieRichardsArm
Copy link
Contributor Author

-        if (A.couldMatchAmbiguouslyWith(B, Target)) {
+        bool PreferSmallerInstructions =
+            AsmParser->getValueAsBit("PreferSmallerInstructions");
+        if (A.couldMatchAmbiguouslyWith(B, PreferSmallerInstructions)) {

@s-barannikov: Same for shouldBeMatchedBefore above.

Fixed.

@AlfieRichardsArm
Copy link
Contributor Author

#83436 (comment)

I would add to this comment

(a) that this flag is useful for Arm (so that people who only know other architectures have an idea why it needs to be here in the first place)

Adding this now

(b) what the preference order is if this flag is false. (Surely the only reason this flag exists, instead of just being always-true, is so that somebody can deliberately set it to false, in which case they have some reason to want a different preference order, and will need to know how to get it.)

The main reason for this flag is this change breaks a lot of assumptions that other backends (MIPS primarily) rely upon (see all the assumptions it broke in the ARM backend to get an idea (: ). I will add a comment to explain the default ordering and the change this makes.

Copy link
Collaborator

@statham-arm statham-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this from the point of view of it filling in the missing comment I asked for on review #83436. I can't comment on whether the details of the specified preference order are accurate, though, so I have to rely on other reviewers for that :-)

@AlfieRichardsArm AlfieRichardsArm merged commit 6854f6f into llvm:main Mar 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants