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

x64: Lower fcopysign, ceil, floor, nearest, and trunc in ISLE #4730

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Aug 17, 2022

Add tests for fcopysign, ceil, floor, nearest, and trunc, and lower all of those instructions in ISLE.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Aug 17, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "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.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

r+ on new commits; will approve once earlier PRs merge and this is unstacked.

Commentary below on rule ordering but I don't think we need to do anything about it now, given context!

(rule (lower (has_type (use_sse41) (ceil a @ (value_type $F32))))
(x64_roundss a (RoundImm.RoundUp)))

(rule (lower (ceil a @ (value_type $F32)))
Copy link
Member

Choose a reason for hiding this comment

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

Here we're implicitly relying on the rule-ordering heuristic again (that pesky #4717!): this rule doesn't do anything to guard on not SSE4.1 and it's not clear to me that the sequence of PatternInsts is "less specific" and shadowed by the above rule without going through the compilation in my head... let's leave this for now though as we are likely to resolve #4717 soon.

@elliottt elliottt force-pushed the trevor/x64-copysign-ceil-floor-nearest-trunc branch from ff513b4 to 9a49520 Compare August 18, 2022 20:21
@elliottt elliottt force-pushed the trevor/x64-copysign-ceil-floor-nearest-trunc branch from 9a49520 to ef97072 Compare August 19, 2022 04:12
@elliottt elliottt marked this pull request as ready for review August 19, 2022 04:13
@elliottt elliottt requested a review from cfallin August 19, 2022 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants