Skip to content
This repository was archived by the owner on May 7, 2024. It is now read-only.

[bitmanip][WiP] [RFC] Add automatic generation of pack* - #262

Closed
rdolbeau wants to merge 4 commits into
riscvarchive:riscv-gcc-10.2.0-rvbfrom
rdolbeau:riscv-gcc-10.2.0-rvb-pack
Closed

[bitmanip][WiP] [RFC] Add automatic generation of pack*#262
rdolbeau wants to merge 4 commits into
riscvarchive:riscv-gcc-10.2.0-rvbfrom
rdolbeau:riscv-gcc-10.2.0-rvb-pack

Conversation

@rdolbeau

@rdolbeau rdolbeau commented May 16, 2021

Copy link
Copy Markdown

This adds automatic generation of pack* instructions (pack, packu, packh) beyond zero-extension.
Unfortunately it's very ad-hoc at the moment.

I'm using the peephole2 pass, but it seems very restrictive in what it matches and perhaps not the best solution.
Any suggestion of where/how to implement this properly welcome.

This has not been tested very much and might be buggy, this is for discussion on how to best leverage B in GCC.
[edit] also currently it's really only for RV32.

The attached pack-pattern.txt is a C file with some C code pattern that should be recognized to generate pack[,u,h]. Some (not all) are identified by this patch. Some of those patterns are identified in real-life codes and the appropriate instruction[s] is/are generated (with RV32IMAFDCK_ZBR_ZBT as the march), such as in Cairo 1.17.4 (cairo-1.17.4/src/cairo-image-info.c) or Poppler 21.05.0 (poppler-21.05.0/fofi/FoFiTrueType.cc) - both are mostly packh/packh/pack to assemble 4 bytes in a word (matching a very ad-hoc pattern...).

@clairexen any suggestion on what packages should have pack*-suitable code patterns? [edit] for rv32; the Skia examples use uint32_t so only need pack* in rv64.

[edit] updated version of the pattern file (4x)
pack-pattern.txt

@kito-cheng kito-cheng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you or SiPearl has signed FSF copyright assignment? If not, could you signed that to prevent any copyright issue in future :)

Comment thread gcc/config/riscv/bitmanip.md Outdated
Comment thread gcc/config/riscv/bitmanip.md Outdated
Comment thread gcc/config/riscv/bitmanip.md Outdated
Comment thread gcc/config/riscv/peephole_pack.md Outdated
})

;; Does it look ad-hoc? I think it looks ad-hoc...
(define_peephole2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe those ad-hoc pattern could be removed once pack* pattern has describe right :)
Or...at least could be simplified.

@rdolbeau rdolbeau May 17, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Actually they still seem necessary...

In fact I had to add some new ones after going for define_inst_and_split, as GCC would sometimes split the two instructions in the pattern, so that's more different scheduling to deal with (maybe the previous ones are obsolete now, not sure how to tell which peepholes has been applied in which functions...).

@rdolbeau

Copy link
Copy Markdown
Author

@kito-cheng I originally tried without unspec, but couldn't get it to reliably match/be matched at the time, so fell back on a lot of unspec.

I've never worked extensively with GCC's MD, and the little I did was 15 years ago :-) I'm sure there's cleaner/better way to do this, which is the reason for this PR; a starting point to get pack* supported properly in GCC.

A lot of the 'ad-hoc-ness' comes from the need to match a lot of instructions to identify the pattern, to make sure only 8 bits are valid/used. The one case that currently doesn't work is uint8_t as argument to a function; as far as I can tell there's not way to tell in the back-end that the 3 MSbytes of the inputs are 0 - that information is already gone. Load bytes work, and seems to be what appear in actual libraries.

And no I don't have a copyright assignment with the FSF at this time.

@rdolbeau

rdolbeau commented May 17, 2021

Copy link
Copy Markdown
Author

@kito-cheng Something like 5fd0898 ? (only for UNSPEC_PACK ATM). But I still need the pack-generating peephole, otherwise the instruction is not generated...

[edit] @kito-cheng The change broke the 'bswap-removing' peephole; basically the (non-unspec) pack expect a reg:HI, but in the peephole I have a (match_dup 5), and operand 5 is a reg:SI . Is there some way to tell the peephole those low-order 16 bits are fine?

@rdolbeau
rdolbeau force-pushed the riscv-gcc-10.2.0-rvb-pack branch 2 times, most recently from b6c91a6 to 3fc9121 Compare May 19, 2021 07:43
@rdolbeau
rdolbeau marked this pull request as draft May 19, 2021 12:05
@rdolbeau rdolbeau changed the title [WiP] [RFC] Add automatic generation of pack* [bitmanip][WiP] [RFC] Add automatic generation of pack* May 19, 2021
@rdolbeau

Copy link
Copy Markdown
Author

@kito-cheng While the 'pack' stuff is too complicated for its own good, maybe c758e92 (which only implement support for sext.[bh]) is worth cherry-picking - and simple enough that the lack of copyright assignment is not a problem? (raised the subject internally, shouldn't be a problem, but might take a while...).

@rdolbeau
rdolbeau force-pushed the riscv-gcc-10.2.0-rvb-pack branch from 0f9b12c to 7e8e7ca Compare May 26, 2021 07:05
@rdolbeau
rdolbeau force-pushed the riscv-gcc-10.2.0-rvb-pack branch from 7e8e7ca to 20192d4 Compare May 26, 2021 07:16
This adds automatic generation of pack* instructions (pack, packu, packh) beyond zero-extension.
This is implemented via a custom pass that
a) reorganize chains of '[ix]or' to exhibit regular patterns;
b) matches common pattern of pack/packu/packh and replace them by the appropriate instruction
@rdolbeau
rdolbeau force-pushed the riscv-gcc-10.2.0-rvb-pack branch 2 times, most recently from 4ff40f6 to 864c008 Compare May 28, 2021 08:04
@rdolbeau
rdolbeau force-pushed the riscv-gcc-10.2.0-rvb-pack branch from 864c008 to 6496add Compare May 28, 2021 08:35
@rdolbeau

rdolbeau commented May 28, 2021

Copy link
Copy Markdown
Author

This has diverged quite a bit from the original code (from peepholes to a specific pass earlier in the process), I'll close it and reopen a new one (#267).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants