riscvarchive / riscv-code-size-reduction

https://jira.riscv.org/browse/RVG-122
151 stars 34 forks source link

hca: Encoding field 16 bit even for 32 bit push instructions #95

Open kjetilos opened 3 years ago

kjetilos commented 3 years ago

Hi,

When analyzing some code with the hca script I see the following output.

{'PC': '8fe29930', 'Encoding': '8436', 'WoE': 32, 'Instruction': 'push', 'Category': 'alu', 'Destination': ('s0',), 'Source': ('ra', 's0-s5')} {'PC': '8fe29e10', 'Encoding': '13612823', 'WoE': 32, 'Instruction': 'push', 'Category': 'store', 'Source': ('ra', 's0-s6'), 'Immediate': '304'} {'PC': '8fe29f84', 'Encoding': '8432', 'WoE': 32, 'Instruction': 'push', 'Category': 'alu', 'Destination': ('s0',), 'Source': ('ra', 's0-s8')} {'PC': '8fe2a0ce', 'Encoding': '15912a23', 'WoE': 32, 'Instruction': 'push', 'Category': 'store', 'Source': ('ra', 's0-s9'), 'Immediate': '340'} {'PC': '8fe2a2aa', 'Encoding': 'dbe6', 'WoE': 32, 'Instruction': 'push', 'Category': 'store', 'Source': ('ra', 's0-s9'), 'Immediate': '244'} {'PC': '8fe2a41e', 'Encoding': '15912a23', 'WoE': 32, 'Instruction': 'push', 'Category': 'store', 'Source': ('ra', 's0-s9'), 'Immediate': '340'} {'PC': '8fe2a5d8', 'Encoding': 'd2a6', 'WoE': 32, 'Instruction': 'push', 'Category': 'store', 'Source': ('ra', 's0-s1'), 'Immediate': '100'}

Notice that these instructions are all "push" instructions and they are all 32 bit, however the Encoding field seems to be 16 bit in 4 of the cases above. Does the encoding field from the hca script follow this specification? https://github.com/riscv/riscv-code-size-reduction/blob/master/ISA%20proposals/Huawei/Zce_spec.adoc#pushpoppopret

tariqkurd-repo commented 3 years ago

Hi @kjetilos the encoding field is invalid in the case that the instruction has been added by the script and should be clearly marked as such.

@abukharmeh please fix this.

Tariq

abukharmeh commented 3 years ago

Hi Kjetilos,

No the script does not attempt to replace the encoding field with the correct value, it always sets the correct WoE and Instruction for size estimation. Some times it sets additional fields like source and destinations but it never sets the correct encoding.

I think it make sense to mark modified instructions fields with XXXX where they are not replaced to indicate that some parts of the instruction were replaced by an optimization scan, but this is not done at the moment.

Also please note that when you wrote your actions for the script call last git issue, I noticed that you call pushpop twice, once with pushpop then with pushpop,both. You only need to call it once with pushpop,both, the script should then decide what is the best use of the encoding and modify instructions accordingly. I have a feeling that you call pushpop before pushpop,both thus the script replaces all calls with 32bits calls.

Kind regards, Ibrahim.

kjetilos commented 3 years ago

Hi Ibrahim,

This explains the Encoding behavior. I will ignore the content of this field for the push and pop instructions.

It sounds like the order of input arguments is important to analyze code size savings correctly with the script. What is the full commandline we should use to measure the total codesize benefits of the Zce extension? Should we only use "pushpop,both" or should we place pushpop after pushpop,both.

abukharmeh commented 3 years ago

Yes the order of the actions is important as they are executed in the same order they are written, and if an action expects instructions that another action changed, then it wont be able to match to them.

You only need to use pushpop,both. I think the way pushpop is splitted now is not the best it can be and I have a patch that slightly improve its behaviour ready to be released, however I am delaying it a bit to make sure I dont have any side effects with #97