Skip to content

[CIR] Use ComplexRealOp for RValue __real__ operator #1689

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmrDeveloper
Copy link
Member

Update __real__ operation to use ComplexRealOp and act directly on the complex value.

Ref: llvm/llvm-project#144235 (review)

@AmrDeveloper
Copy link
Member Author

CC. @andykaylor

Copy link
Collaborator

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

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

lgtm

// CHECK-AFTER: }

// LLVM: define dso_local void @extract_real()
// LLVM: %{{.+}} = load double, ptr @c, align 8
// LLVM: %{{.+}} = load i32, ptr @ci, align 4
// LLVM: %[[COMPLEX_D:.*]] = load { double, double }, ptr @c, align 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not great that we're loading the whole complex value when we only need one component, but hopefully the optimizer will do something about that. If necessary, we can create a CIR-specific transform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, we can check if it is handled by the optimiser, or we can create our own transform. I will create a PR for __imag__ :D

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Is cir.complex.real_ptr used at all after this PR? If so the operation should also be removed

@AmrDeveloper
Copy link
Member Author

Is cir.complex.real_ptr used at all after this PR? If so the operation should also be removed

Yes cir.complex.real_ptr is used in case __real__ used in RValue for example

__real__ c = 10;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants