[TableGen] fix tlbgen for EncodingByHwMode

[superZWT123]

By using 'HwMode', we can let one instruction have different encodings. But there are some defects in the original TableGen logic in llvm framework.

1. If we use HwModes to control one instruction's encodings, there will generate multiple copies of the original DecoderTables to where the instruction belongs. But for instructions who are not controlled by HwModes, tablegen still generate multiple copies of DecoderTables for them, which is an absolutely redundant behavior.
2. If we use HwModes to control one instruction's encodings, when get binary code for instruction, framework will check all subtargets' HwModes, but we only set HwModes for subtargets whose instructions' encodings conflict, those subtargets without HwModes will cause 'llvm_unreachable'.
3. RegInfoByHwMode and EncodingByHwModes cannot coexist, and resolve it by store HwModes Id in bits.
4. Fix DefaultMode logic for EncodingByHwMode in Decoder and CodeEmitter.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[superZWT123]

For more details, please refer to： https://discourse.llvm.org/t/rfc-fix-tablegen-for-hwmode/77625

[superZWT123]

@nvjle @topperc Hello, could you please help me review my code.

[kparzysz]

This makes sense to me, but I'd wait for someone else to review this to see if it meets their needs.

A couple of suggestions regarding style:

• use structured bindings in place of "auto &KV : some-map",
• modify "append" to take "const std::string &", it would get rid of the "c_str" and make the code easier to read.

[kparzysz]

Typo: dosen't -> doesn't.

[superZWT123]

Done fix.

Incremental changes ：https://github.com/llvm/llvm-project/compare/15ee7104cef3344c605181e1ab211c91148605f3..629b8fdef10031ee5f5588e10487e1e364ac81ec

[nvjle]

@nvjle @topperc Hello, could you please help me review my code.

@superZWT123 Please give me a day or two to review this code. From a very quick scan, it actually undoes some of the work I did recently (or at least does it differently)-- which may be fine-- but I need to examine carefully. I'm using this feature in a downstream production backend with a complicated ISA (generously speaking) and even more complicated encoding schemes (100+ decoder tables). It tends to exercise the encode/decode heavily.

Also, my previous patch to suppress duplicates had a command line option so that existing downstream backends could opt-in to that feature-- at least temporarily. That is, a downstream backend could rebase and their disassembler would not need any changes due to decoder table naming. Something like that would probably be good to keep initially.

All of that said, I am certainly glad to see others helping to production-harden this feature.

[superZWT123]

This makes sense to me, but I'd wait for someone else to review this to see if it meets their needs.

A couple of suggestions regarding style:

• use structured bindings in place of "auto &KV : some-map",
• modify "append" to take "const std::string &", it would get rid of the "c_str" and make the code easier to read.

Thanks kparzysz, I can’t assert that my modification is the optimal solution, but what I can say with certainty is that this logic is definitely better than before and can solve many current problems. One important point is that the current changes don’t require any modifications from the backends that are already using hwmode, offering good compatibility.

[superZWT123]

@nvjle @topperc Hello, could you please help me review my code.

@superZWT123 Please give me a day or two to review this code. From a very quick scan, it actually undoes some of the work I did recently (or at least does it differently)-- which may be fine-- but I need to examine carefully. I'm using this feature in a downstream production backend with a complicated ISA (generously speaking) and even more complicated encoding schemes (100+ decoder tables). It tends to exercise the encode/decode heavily.

Also, my previous patch to suppress duplicates had a command line option so that existing downstream backends could opt-in to that feature-- at least temporarily. That is, a downstream backend could rebase and their disassembler would not need any changes due to decoder table naming. Something like that would probably be good to keep initially.

All of that said, I am certainly glad to see others helping to production-harden this feature.

Thanks nvjle,
I recently reviewed your four changes related to hwmode. Your two modifications to the DecoderTable (“fix IndexOfInstruction” and “fix EncodingId and Opcode mismatch”) were similar to changes I had implemented locally. However, after comparison, I realized your changes are significantly superior to mine, particularly in terms of time complexity optimization. For the table duplication fix, I believe my modification might be more advantageous. My changes could drastically reduce table duplication and minimize the complexity of adapting the backend to accommodate EncodingByHwMode changes.

If you could help with validating my patch on your backend, that would be fantastic. Finally, I am truly thankful for the time you spent reviewing my code!

[github-actions]

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

[superZWT123]

@nvjle @topperc Hello, could you please help me review my code.

@superZWT123 Please give me a day or two to review this code. From a very quick scan, it actually undoes some of the work I did recently (or at least does it differently)-- which may be fine-- but I need to examine carefully. I'm using this feature in a downstream production backend with a complicated ISA (generously speaking) and even more complicated encoding schemes (100+ decoder tables). It tends to exercise the encode/decode heavily.

Also, my previous patch to suppress duplicates had a command line option so that existing downstream backends could opt-in to that feature-- at least temporarily. That is, a downstream backend could rebase and their disassembler would not need any changes due to decoder table naming. Something like that would probably be good to keep initially.

All of that said, I am certainly glad to see others helping to production-harden this feature.

Hi nvjle.
I hope all is well with you. Since you mentioned needing a day or two to carefully review my code, three days have now passed, and I’m curious if you’ve had the opportunity to complete your review. Are there any suggestions or adjustments you’d like me to make? If not, may I proceed with merging my code into the main branch?
I look forward to your valuable feedback. Thanks!

[nvjle]

@nvjle @topperc Hello, could you please help me review my code.

@superZWT123 Please give me a day or two to review this code. From a very quick scan, it actually undoes some of the work I did recently (or at least does it differently)-- which may be fine-- but I need to examine carefully. I'm using this feature in a downstream production backend with a complicated ISA (generously speaking) and even more complicated encoding schemes (100+ decoder tables). It tends to exercise the encode/decode heavily.
Also, my previous patch to suppress duplicates had a command line option so that existing downstream backends could opt-in to that feature-- at least temporarily. That is, a downstream backend could rebase and their disassembler would not need any changes due to decoder table naming. Something like that would probably be good to keep initially.
All of that said, I am certainly glad to see others helping to production-harden this feature.

Hi nvjle. I hope all is well with you. Since you mentioned needing a day or two to carefully review my code, three days have now passed, and I’m curious if you’ve had the opportunity to complete your review. Are there any suggestions or adjustments you’d like me to make? If not, may I proceed with merging my code into the main branch? I look forward to your valuable feedback. Thanks!

Hi @superZWT123,
I've done some reviewing and done a trial incorporation into a production downstream backend.
To summarize briefly, there are some significant problems with this patch in its current form. The first problem is that, in the mentioned downstream backend with these decoder changes applied, all decoder duplicate suppression is reverted compared to the existing duplicate suppression. This definitely can not be merged as is. The second problem is that with the encoding changes, I'm seeing miscompiles-- but I've not had time to investigate precisely what is happening there (enormous 200,000+ line TD files, complicated encoding schemes, binary diffs-- TBD).

Patches to the decode and encode can be subtle-- especially in larger backends. Here are some specific requests and observations:

1. Typically in LLVM we adhere to this policy for submitting patches: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch . Specifically, independent/orthogonal changes should be separate patches. This patch incorporates two completely independent issues (even though both rely on EncodingByHwMode) and each is subtle and needs to be considered separately. In this patch, I would respectfully request that it be split into two orthogonal parts-- one for the CodeEmitterGen+SubtargetEmitter+RegisterInfoEmitter set of changes and the other for the DecoderEmitter changes. This will allow detailed attention to each (which they need) and independent timelines for potential merging of each (and likewise for downstream backends-- they can independently incorporate/test/revert orthogonal changes). In any case, I'll give some initial thoughts below which can go into the follow-up patches preemptively for quicker turnaround.
2. The decoder emitter changes here essentially undo/cancel much of the existing support for duplicate suppression (in our backend, 100% of duplicates are no longer detected[1]). It also undoes the command line option to opt out of suppression. This means any existing downstream backend that already relies on EncodingByHwMode may need to have its decoder tables altered. It is better in the short term to allow targets to decide whether or not to apply suppression-- particularly in production backends who are necessarily conservative or just want to change on their own timeline (consider downstream backends who pull-in upstream changes daily/weekly on an automated basis). This can be further discussed in a separate decoder patch, but at the moment, this is significantly worse than that current state of affairs. The good news is, I think by merely using "" (empty name) instead of AllModes as the shared/common decoder table name in the existing code (trivial change, and use -gen-disassembler --suppress-per-hwmode-duplicates), you would have the effect you are trying to achieve. I originally used that name to make it explicit in the generated code that the table applies to all instructions that do not have a per-HwMode override, but a blank name is perfectly acceptable too-- specifically for the case when a backend did not previously have EncodingByHwMode, but will add it. [1] For example, in unit test HwModeEncodeDecode2.td (which was carefully constructed to show/test duplicate suppression), instruction bar should only appear in one table after suppression-- and indeed that is true with the current suppression code in TOT. Your patch regresses that case and leaves bar duplicated. The current TOT code seems to entirely obviate the decoder part of the patch (save the AllModes name change if it is desired).
3. I see some obvious inefficiencies and coding standard nits (https://llvm.org/docs/CodingStandards.html) that you could attend to while splitting the patch. For example, in the generated getRegInfosHwModeId, there is an entire loop nest to simply detect that a single bit is set. That can be replaced entirely with a single bitwise expression (or use llvm::has_single_bit + zero check). A number of comments need to be updated (all comments should be standard prose/caps/full-stop/etc).
4. A simple TableGen unit test is needed for the reginfo/subtarget-emitters changes. Also, I think it might make sense to have two different APIs for this since getHwMode can now return a set. Maybe the original for callers who expect exactly one live mode at a given moment (the original intent, as far as I know), and a second getHwModeSet for callers expecting a bit vector of live modes. If not that, at the very least, some comments need to be added explaining what this API now returns for future readers of this and related code.

I would say 3 days of review time is not terrible in LLVM time, especially for this sort of patch. I will certainly try to turn around as quickly as possible-- with the emitter changes being likely quicker to turnaround.

FWIW, in recent changes to TableGen, I've been very careful myself about making narrow, isolated changes. This helps reviewers more easily understand and review the code (lessen cognitive load for people who largely work full time and need to context switch). It also makes it easier to pinpoint any problems later and revert/change relatively small amounts of code. This does require a bit more effort to upstream patches from downstream/private backends, but this is exactly what all of us "living downstream" are doing.

[superZWT123]

@nvjle @topperc Hello, could you please help me review my code.

@superZWT123 Please give me a day or two to review this code. From a very quick scan, it actually undoes some of the work I did recently (or at least does it differently)-- which may be fine-- but I need to examine carefully. I'm using this feature in a downstream production backend with a complicated ISA (generously speaking) and even more complicated encoding schemes (100+ decoder tables). It tends to exercise the encode/decode heavily.
Also, my previous patch to suppress duplicates had a command line option so that existing downstream backends could opt-in to that feature-- at least temporarily. That is, a downstream backend could rebase and their disassembler would not need any changes due to decoder table naming. Something like that would probably be good to keep initially.
All of that said, I am certainly glad to see others helping to production-harden this feature.

Hi nvjle. I hope all is well with you. Since you mentioned needing a day or two to carefully review my code, three days have now passed, and I’m curious if you’ve had the opportunity to complete your review. Are there any suggestions or adjustments you’d like me to make? If not, may I proceed with merging my code into the main branch? I look forward to your valuable feedback. Thanks!

Hi @superZWT123, I've done some reviewing and done a trial incorporation into a production downstream backend. To summarize briefly, there are some significant problems with this patch in its current form. The first problem is that, in the mentioned downstream backend with these decoder changes applied, all decoder duplicate suppression is reverted compared to the existing duplicate suppression. This definitely can not be merged as is. The second problem is that with the encoding changes, I'm seeing miscompiles-- but I've not had time to investigate precisely what is happening there (enormous 200,000+ line TD files, complicated encoding schemes, binary diffs-- TBD).

Patches to the decode and encode can be subtle-- especially in larger backends. Here are some specific requests and observations:

1. Typically in LLVM we adhere to this policy for submitting patches: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch . Specifically, independent/orthogonal changes should be separate patches. This patch incorporates two completely independent issues (even though both rely on EncodingByHwMode) and each is subtle and needs to be considered separately. In this patch, I would respectfully request that it be split into two orthogonal parts-- one for the CodeEmitterGen+SubtargetEmitter+RegisterInfoEmitter set of changes and the other for the DecoderEmitter changes. This will allow detailed attention to each (which they need) and independent timelines for potential merging of each (and likewise for downstream backends-- they can independently incorporate/test/revert orthogonal changes). In any case, I'll give some initial thoughts below which can go into the follow-up patches preemptively for quicker turnaround.
2. The decoder emitter changes here essentially undo/cancel much of the existing support for duplicate suppression (in our backend, 100% of duplicates are no longer detected[1]). It also undoes the command line option to opt out of suppression. This means any existing downstream backend that already relies on EncodingByHwMode may need to have its decoder tables altered. It is better in the short term to allow targets to decide whether or not to apply suppression-- particularly in production backends who are necessarily conservative or just want to change on their own timeline (consider downstream backends who pull-in upstream changes daily/weekly on an automated basis). This can be further discussed in a separate decoder patch, but at the moment, this is significantly worse than that current state of affairs. The good news is, I think by merely using "" (empty name) instead of AllModes as the shared/common decoder table name in the existing code (trivial change, and use -gen-disassembler --suppress-per-hwmode-duplicates), you would have the effect you are trying to achieve. I originally used that name to make it explicit in the generated code that the table applies to all instructions that do not have a per-HwMode override, but a blank name is perfectly acceptable too-- specifically for the case when a backend did not previously have EncodingByHwMode, but will add it. [1] For example, in unit test HwModeEncodeDecode2.td (which was carefully constructed to show/test duplicate suppression), instruction bar should only appear in one table after suppression-- and indeed that is true with the current suppression code in TOT. Your patch regresses that case and leaves bar duplicated. The current TOT code seems to entirely obviate the decoder part of the patch (save the AllModes name change if it is desired).
3. I see some obvious inefficiencies and coding standard nits (https://llvm.org/docs/CodingStandards.html) that you could attend to while splitting the patch. For example, in the generated getRegInfosHwModeId, there is an entire loop nest to simply detect that a single bit is set. That can be replaced entirely with a single bitwise expression (or use llvm::has_single_bit + zero check). A number of comments need to be updated (all comments should be standard prose/caps/full-stop/etc).
4. A simple TableGen unit test is needed for the reginfo/subtarget-emitters changes. Also, I think it might make sense to have two different APIs for this since getHwMode can now return a set. Maybe the original for callers who expect exactly one live mode at a given moment (the original intent, as far as I know), and a second getHwModeSet for callers expecting a bit vector of live modes. If not that, at the very least, some comments need to be added explaining what this API now returns for future readers of this and related code.

I would say 3 days of review time is not terrible in LLVM time, especially for this sort of patch. I will certainly try to turn around as quickly as possible-- with the emitter changes being likely quicker to turnaround.

FWIW, in recent changes to TableGen, I've been very careful myself about making narrow, isolated changes. This helps reviewers more easily understand and review the code (lessen cognitive load for people who largely work full time and need to context switch). It also makes it easier to pinpoint any problems later and revert/change relatively small amounts of code. This does require a bit more effort to upstream patches from downstream/private backends, but this is exactly what all of us "living downstream" are doing.

Hi nvjle, I have carefully read your comments and discussed your suggestions with our team members. Thank you very much for taking the time to review this patch and providing valuable feedback.
I also work for a production downstream backend. Our backend has twenty to thirty subtargets, so the instructions' definitions in our backend are quite complex, and there are many outdated instruction definitions that are difficult to maintain. So we are very much looking forward to enabling the ‘EncodingByHwMode’ feature to release the pressure on our backend's instructions' definitions. It is foreseeable that with the development of LLVM, more and more dowmstream backend may face the same problem (instruction definition inflation).

• For your first point, I will split my patch into two parts, one is about the changes of DecoderEmitter(eliminate table duplication) and the other is about storing HwModes in bits form and sinking HwModes selection logic down to per instructions level.

• For your second point, I will keep the '--suppress-per-hwmode-duplicates' option, allowing downstream backends who already used this option to decide whether or not to apply suppression.

• For your third point, I will consider using your solution to handle bitwise related operations, if it can reduce time complexity.

• For your fourth point, after our discussion, we have decided to keep the current approach because it is compatible with the previous approach, and adding an new interface may seem a bit redundant in our opinion. In fact, the old logic cannot detect that a subtarget has two hwmode attributes, which is actually a bug. Of course, I will add more comments to describe the changes to this interface and specific testcases for this feature will be added.

What worries me the most is what you mentioned about seeing miscompiles in your backend. If you can provide some error information, I believe I could better pinpoint where the problem lies. Alternatively, after I split the patch into two parts, it might be easier to reproduce and locate the problem on your backend, but this would probably require your continued assistance in reviewing the split patches. In fact, I enabled the current patch in my backend and assigned the 'EncodingByHwMode' attribute to seven or eight instructions to resolve encoding conflicts. So far, I have not encountered any problems.

Also, I wanted to ask if I should split this pull request into two new pull requests, or just split the commit of this pull request into two? Since this branch has already fallen significantly behind the main branch, I might rebase it and then resubmit two new pull requests. I’m not sure if this is the correct approach.

If there are any points that I haven't replied to, please point them out. Thank you very much!

[nvjle]

Hi @superZWT123 ,

What worries me the most is what you mentioned about seeing miscompiles in your backend. If you can provide some error information, I believe I could better pinpoint where the problem lies. Alternatively, after I split the patch into two parts, it might be easier to reproduce and locate the problem on your backend, but this would probably require your continued assistance in reviewing the split patches. In fact, I enabled the current patch in my backend and assigned the 'EncodingByHwMode' attribute to seven or eight instructions to resolve encoding conflicts. So far, I have not encountered any problems.

As mentioned earlier, I have not yet had time to nail down precisely what is happening here, and whether related to the encoder changes per-se or something on our end. It has taken all of my time so far just integrating and reviewing all of the changes-- another reason independent/small changes are important. I should have this nailed down soon.

Also, I wanted to ask if I should split this pull request into two new pull requests, or just split the commit of this pull request into two? Since this branch has already fallen significantly behind the main branch, I might rebase it and then resubmit two new pull requests. I’m not sure if this is the correct approach.

Submitting two new pull requests (after rebasing) is what I had in mind. Two logically independent patches which can be reviewed separately and on different timelines.

If there are any points that I haven't replied to, please point them out. Thank you very much!

Regarding the DecoderEmitter part of the patch: One crucial point not touched on above (but this should be discussed in the follow-up decoder-related PR)-- is that decoder suppression already exists in TOT, and this part of the patch seems entirely unaware of that. Indeed it essentially just cancels/undoes some of the existing support (as if the merge conflict was just resolved in favor of downstream without consideration of the existing functionality).
This isn't just the command line option, but the actual suppression code. See the bar example in HwModeEncodeDecode2.td pointed to earlier, where the current patch regresses. That unit test was deliberately constructed to make sure suppression occurs-- but this patch removed that as well. As mentioned above, it appears that the decoder patch is trying to achieve what TOT already does (and TOT does more thoroughly and cleanly)-- just with a different (blank) name for the AllModes tables. This is what I meant above when I said "The current TOT code seems to entirely obviate the decoder part of the patch". The follow-up decoder patch (if needed at all) should address those points: explain what is meant to be achieved relative to TOT; how/why TOT does not already achieve that aim; ensure no suppression regressions.

[superZWT123]

Regarding the DecoderEmitter part of the patch: One crucial point not touched on above (but this should be discussed in the follow-up decoder-related PR)-- is that decoder suppression already exists in TOT, and this part of the patch seems entirely unaware of that. Indeed it essentially just cancels/undoes some of the existing support (as if the merge conflict was just resolved in favor of downstream without consideration of the existing functionality). This isn't just the command line option, but the actual suppression code. See the bar example in HwModeEncodeDecode2.td pointed to earlier, where the current patch regresses. That unit test was deliberately constructed to make sure suppression occurs-- but this patch removed that as well. As mentioned above, it appears that the decoder patch is trying to achieve what TOT already does (and TOT does more thoroughly and cleanly)-- just with a different (blank) name for the AllModes tables. This is what I meant above when I said "The current TOT code seems to entirely obviate the decoder part of the patch". The follow-up decoder patch (if needed at all) should address those points: explain what is meant to be achieved relative to TOT; how/why TOT does not already achieve that aim; ensure no suppression regressions.

Thank you very much for your response nvjle, which I saw in the middle of the night. Regarding your third point, I roughly understand what you mean. You meant that the suppression was already implemented in your previous patch, and there is no need to repeat that. I have carefully considered and compared the implementation of our two patches, and I will explain my specific ideas.

Firstly, if we define two hwmodes(Mode1 and Mode2) for instruction A, the old logic will split all DecoderTables into two form, each corresponding to one of the two hwmodes. Except for the DecoderTable containing instruction A, the rest of the tables do not need to be duplicated and as far as I'm concerned must be preserved in their original form. My patch accomplishes this by removing unnecessary table duplications and retaining necessary table duplications for hwmode enablement.

As for your patch, you first add an ‘AllModes’ suffix to the DecoderTables that do not need any modification(no hwmodes in them), which is unnecessary in my opinion. Leaving that aside, instruction A is then individually selected, and two DecoderTables, one for hwmode 1 and another for hwmode 2, are created storing the different encodings infos for instruction A. All the other instructions under the same 'DecoderNamespace' but without the hwmode attribute as instruction A are saved in an 'AllModes' table. Overall, I see that your suppression method separates the instructions assigned with hwmode attributes from their original 'DecoderNamespace', which is not suitable since hwmode should not change the DecoderNamespace of a given instruction.

Once you extract hwmode-specific instructions using your approach, you will no longer be able to check whether the encoding of instruction A under hwmode 1 and hwmode 2 conflicts when disassembling with other instructions that belong to the same DecoderNamespace.

Actually I have considered extracting instructions with hwmode attributes into separate tables before I seeing your patch, this can minimize table duplications to the greatest extent possible, but I felt that this way might break the logic of DecoderNamespace, so I just give it up. My view is that if users do not specify a new DecoderNamespace, the encodings of instructions under different hwmodes should still remain within the previous DecoderNamespace.

Of course, your suppression method is more radical and can significantly reduce the size of the DecoderTable if users are made aware of it and knowing completely what this command line is doing. But still want to say , if you can remove the ‘AllModes’ suffix , it would be better.

[superZWT123]

Hi nvjle, kparzysz. I might take a few days to organize my patch and perform some more in-depth checks (such as enabling the EncodingByHwMode functionality on other backends like RISCV, and build tablegen in debug mode to dump some records) to ensure the functionality is correct, although we have already conducted numerous CI tests on this patch in our local project. I will try to decouple the code of 'DecoderEmitter' part within one or two days to submit a pull request first. Thank you very much for your patient review and valuable comments.

[nvjle]

Hi nvjle, kparzysz. I might take a few days to organize my patch and perform some more in-depth checks (such as enabling the EncodingByHwMode functionality on other backends like RISCV, and build tablegen in debug mode to dump some records) to ensure the functionality is correct, although we have already conducted numerous CI tests on this patch in our local project. I will try to decouple the code of 'DecoderEmitter' part within one or two days to submit a pull request first. Thank you very much for your patient review and valuable comments.

Sounds good @superZWT123, and doing the DecoderEmitter part first makes sense. From your paragraph earlier, you essentially want a less aggressive suppression compared to what exists in TOT today. That suggests changing the existing boolean suppress option to an integer option which has two modes, e.g. 0 for existing suppression, and 1 for less aggressive suppression (being an obscure option of TableGen, a simple integer option is fine). That preserves the existing code and functionality while also adding the less aggressive form. Also, with respect to removing the existing AllModes suffix of the current suppression, I have no objection.