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

handle multiple optional operands #81

Merged
merged 15 commits into from
Jul 23, 2024
Merged

handle multiple optional operands #81

merged 15 commits into from
Jul 23, 2024

Conversation

jumerckx
Copy link
Collaborator

for example openacc.enter_data:

function enter_data(ifCond=nothing::Union{Nothing, Value}; asyncOperand=nothing::Union{Nothing, Value}, waitDevnum=nothing::Union{Nothing, Value}, waitOperands::Vector{Value}, copyinOperands::Vector{Value}, createOperands::Vector{Value}, createZeroOperands::Vector{Value}, attachOperands::Vector{Value}, async=nothing, wait=nothing, location=Location())
    results = IR.Type[]
    operands = Value[waitOperands..., copyinOperands..., createOperands..., createZeroOperands..., attachOperands..., ]
    owned_regions = Region[]
    successors = Block[]
    attributes = NamedAttribute[]
    !isnothing(ifCond) && push!(operands, ifCond)
    !isnothing(asyncOperand) && push!(operands, asyncOperand)
    !isnothing(waitDevnum) && push!(operands, waitDevnum)
    push!(attributes, operandsegmentsizes([isnothing(ifCond) ? 0 : 1, isnothing(asyncOperand) ? 0 : 1, isnothing(waitDevnum) ? 0 : 1, length(waitOperands), length(copyinOperands), length(createOperands), length(createZeroOperands), length(attachOperands), ]))
    !isnothing(async) && push!(attributes, namedattribute("async", async))
    !isnothing(wait) && push!(attributes, namedattribute("wait", wait))
    
    IR.create_operation(
        "acc.enter_data", location;
        operands, owned_regions, successors, attributes,
        results=results,
        result_inference=false
    )
end

There's a trailing comma in the list now but that should be no problem.

@mofeing
Copy link
Collaborator

mofeing commented Jul 22, 2024

Do you mind running the formatter? That way we can check out more easily if the bug is solved.

@jumerckx
Copy link
Collaborator Author

I ran the formatter using

format(".", JuliaFormatter.BlueStyle())

and the ternary operators haven't been replaced by if-else statements. Not sure whether that's good or bad.

@mofeing
Copy link
Collaborator

mofeing commented Jul 22, 2024

That's okay: it's how we have it right now.

@jumerckx
Copy link
Collaborator Author

I've now pushed dialects for all versions except 18 because I'm not sure how to actually do that.
There are also some small changes to the libMLIR_h.jls for some reason. I've added them to this pr but can revert that commit if wanted.

Comment on lines 368 to 369
operands::Vector{Value},
attributes::Vector{Value},
Copy link
Collaborator

@mofeing mofeing Jul 23, 2024

Choose a reason for hiding this comment

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

Are these renames intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that change was introduced here #74 to fix e.g.

!isnothing(results) && push!(results, results)

It looks like the dialects for LLVM18 weren't generated using the newest generator.

Copy link
Collaborator

@mofeing mofeing Jul 23, 2024

Choose a reason for hiding this comment

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

Ah yeah, in that case wouldn't be better to prioritize a clean interface? (i.e. to use _results as the variable inside and results as the kwarg).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be the case now.

@mofeing
Copy link
Collaborator

mofeing commented Jul 23, 2024

Actually with just the jl_generator.cc would be ok, since then I can upload it to Yggdrasil and regenerate them correctly.

@jumerckx
Copy link
Collaborator Author

jumerckx commented Jul 23, 2024

Actually with just the jl_generator.cc would be ok

you mean to not yet include the generated dialects for LLVM18? or also get rid of the other versions in this PR?

@mofeing
Copy link
Collaborator

mofeing commented Jul 23, 2024

Exactly, we can generate it with bindings/make.jl once it's up in Yggdrasil.

@jumerckx
Copy link
Collaborator Author

jumerckx commented Jul 23, 2024

Exactly, we can generate it with bindings/make.jl once it's up in Yggdrasil.

For now I've only included the generated files for LLVM14 to be able to inspect, I can remove those before merging.

EDIT: nvm, I added the other versions as well to check CI.

@jumerckx
Copy link
Collaborator Author

Green!

@mofeing
Copy link
Collaborator

mofeing commented Jul 23, 2024

Great! Thanks @jumerckx!

@mofeing mofeing merged commit 1e5e8a2 into main Jul 23, 2024
7 checks passed
@mofeing mofeing deleted the jm/generator_fix branch July 23, 2024 15:19
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.

2 participants