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

Various small fixes #50

Closed
wants to merge 11 commits into from
Closed

Conversation

rmalmain
Copy link
Collaborator

Fixes for some compilation problems I've met.
Also, now docker building is more strict and compiles with TCG debug asserts and werror.

Copy link
Member

@aurelf aurelf left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, can you look at my comments?

tcg/tcg-op-vec.c Outdated
op->args[4] = d;
op->args[5] = e;
}
// static void vec_gen_6(TCGOpcode opc, TCGType type, unsigned vece, TCGArg r,
Copy link
Member

Choose a reason for hiding this comment

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

Can you use C comments like elsewhere ? And add a note on why this is commented out vs removed (I imagine this is unused now but may be useful later ?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an effect of -werror, unused symbols will make the compilation fail (it was treated as warning before).
I'll adapt the comment style and add a comment.
It's better to keep it commented imho for future merges with QEMU, maybe this will become used elsewhere one day.

tcg/tcg-op-vec.c Outdated
}
return true;
}
// static bool do_op2(unsigned vece, TCGv_vec r, TCGv_vec a, TCGOpcode opc)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

tcg/tcg-op-vec.c Outdated
TCGArg bi = temp_arg(bt);
TCGArg ci = temp_arg(ct);
TCGArg di = temp_arg(dt);
// TCGArg ri = temp_arg(rt);
Copy link
Member

Choose a reason for hiding this comment

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

Same

tcg/tcg-op-vec.c Outdated
@@ -1083,7 +1083,7 @@ void tcg_gen_cmpsel_vec(TCGCond cond, unsigned vece, TCGv_vec r,

tcg_assert_listed_vecop(INDEX_op_cmpsel_vec);
hold_list = tcg_swap_vecop_list(NULL);
can = tcg_can_emit_vec_op(INDEX_op_cmpsel_vec, type, vece);
// can = tcg_can_emit_vec_op(INDEX_op_cmpsel_vec, type, vece);
Copy link
Member

Choose a reason for hiding this comment

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

same

Dockerfile Outdated

RUN add-apt-repository ppa:ubuntu-toolchain-r/test -y

RUN apt update && apt install -y \
Copy link
Member

Choose a reason for hiding this comment

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

Actually sync and install on the same line is a good idea no ? e.g.:
https://stackoverflow.com/a/61439544
Keep it on the same line ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. I always group apt udpate and apt install in docker files to avoid the problem mentionned in the link you sent.
The thing you saw in the dockerfile that I reverted was to install the latest version of gcc and llvm for testing. Maybe this is something we may reuse in the future (like for testing with different versions of llvm)?

@aurelf
Copy link
Member

aurelf commented Mar 18, 2024

That sounded familiar, in fact i had #45 for this too :/
I should have merged it earlier...
One difference, didn't you also need to specify "typedef enum uint32_t " in the last chunk of 1fba099 ?

@rmalmain
Copy link
Collaborator Author

Ah didn't know about this.
I didn't use a typedef enum, and just used uint32_t instead of the enum.
Your solution sounds better, I will edit with your proposition.

Make example more simple.
Commented out unused QEMU code.
@rmalmain
Copy link
Collaborator Author

merged with #51.

@rmalmain rmalmain closed this Mar 20, 2024
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