Skip to content

Migrate clz, ctz, popcnt, bitrev, is_null, is_invalid on x64 to ISLE.#3848

Merged
fitzgen merged 1 commit intobytecodealliance:mainfrom
cfallin:isle-bitops
Feb 28, 2022
Merged

Migrate clz, ctz, popcnt, bitrev, is_null, is_invalid on x64 to ISLE.#3848
fitzgen merged 1 commit intobytecodealliance:mainfrom
cfallin:isle-bitops

Conversation

@cfallin
Copy link
Member

@cfallin cfallin commented Feb 25, 2022

Along the way this introduces a few new utility bits to the ISLE bindings, e.g. some constructors to do integer arithmetic on constants (I'm surprised we got this far without it!). The bit-twiddling algorithms are, at least to my eye, a bit more legible in this form, but I'm happy to add more comments here if desired since we're making a pass over everything anyway.

Also updated a few stray places I had missed earlier where we can use implicit conversions (the def_inst removals).

@cfallin cfallin requested review from abrown and fitzgen February 25, 2022 00:49
@cfallin cfallin force-pushed the isle-bitops branch 2 times, most recently from 9a08a95 to e226cd2 Compare February 25, 2022 01:40
@cfallin cfallin changed the title Migrate clz, ctz, and popcnt on x64 to ISLE. Migrate clz, ctz, popcnt, and bitrev on x64 to ISLE. Feb 25, 2022
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Feb 25, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

Details This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@cfallin cfallin changed the title Migrate clz, ctz, popcnt, and bitrev on x64 to ISLE. Migrate clz, ctz, popcnt, bitrev, is_null, is_invalid on x64 to ISLE. Feb 25, 2022
Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I think this looks good to me with some questions below.

@abrown
Copy link
Member

abrown commented Feb 25, 2022

One more thought: it might be worthwhile to check that all of the instructions ported here are covered by runtests.

@cfallin
Copy link
Member Author

cfallin commented Feb 25, 2022

Updated, thanks!

One more thought: it might be worthwhile to check that all of the instructions ported here are covered by runtests.

Yep, I've verified that all of the bitops here are exercised by runtests; they actually caught a few bugs during porting.

@cfallin
Copy link
Member Author

cfallin commented Feb 25, 2022

@abrown you probably want to take another look at the changes related to CmoveOr before I merge?

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

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants