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

support llvm 17 #187

Merged
merged 10 commits into from
Jan 18, 2024
Merged

support llvm 17 #187

merged 10 commits into from
Jan 18, 2024

Conversation

5c4lar
Copy link
Contributor

@5c4lar 5c4lar commented Dec 25, 2023

No description provided.

@vtjnash
Copy link
Member

vtjnash commented Jan 12, 2024

That is quite a large diff. Is that all necessary, or are there formatting changes that can be separated?

@5c4lar
Copy link
Contributor Author

5c4lar commented Jan 13, 2024

That is quite a large diff. Is that all necessary, or are there formatting changes that can be separated?

Yeah, formatting changes can be separated, there are only few necessary changes.

@vtjnash
Copy link
Member

vtjnash commented Jan 13, 2024

That would be helpful then, as this is too big to open and will conflict with everyone's open work, so we probably do not want to merge any unnecessary format changes

@5c4lar
Copy link
Contributor Author

5c4lar commented Jan 14, 2024

That would be helpful then, as this is too big to open and will conflict with everyone's open work, so we probably do not want to merge any unnecessary format changes

Thanks for your reply, I format it with clang-format again and the diff is much smaller now.

@vtjnash
Copy link
Member

vtjnash commented Jan 14, 2024

Seems like more clang-format fixes are still needed?

@5c4lar
Copy link
Contributor Author

5c4lar commented Jan 15, 2024

Linux and windows tests are passed now. The macos tests failed due to the warnings introduced during compilation of some test cases. Maybe more flags should be introduced here.

@vtjnash
Copy link
Member

vtjnash commented Jan 15, 2024

Looks like it used to be disabled in the distant past:

// Disable .loc support for older OS X versions.
if (TheTriple.isMacOSX() && TheTriple.isMacOSXVersionLT(10, 6)) {
}
// TODO: Find a replacement to this function
/* Greg Simpson 6-09-13
no member named setMCUseLoc
removed statement
Target.setMCUseLoc(false); */

I don't know what might be wrong about the current output:

Out << "#line " << Loc->getLine() << " \"" << Directory << Filename
<< "\""
<< "\n";

Do you think you could update the test so that it dumps the contents of the generated file to stderr if the test fails?

@5c4lar
Copy link
Contributor Author

5c4lar commented Jan 18, 2024

I dump the generated code for the failure case and play with it on my mac. It seems that the warning is introduced by -g option, and there is nothing wrong with llvm-cbe output.

To reproduce the warning, you can compile try compile the following simple example.

// /opt/homebrew/bin/gcc-13 -g -O1 test.c
#include <stdint.h>
struct l_array_10_uint32_t
{
    uint32_t array[10];
};

int main() {
  struct l_array_10_uint32_t example;
  int i;
  for (i = 0; i < 10; ++i) {
    example.array[i] = i;
  }
  return example.array[6];
}

I think it would be safe to remove the -g option and make the test pass.

Having the above warning fixed, there are still two warnings regarding uninitialized variables, which is introduced by phi node.

@5c4lar
Copy link
Contributor Author

5c4lar commented Jan 18, 2024

Having the above warning fixed, there are still two warnings regarding uninitialized variables, which is introduced by phi node.

Tests passed, the version of gcc is different in the github actions :)

@vtjnash vtjnash merged commit 5a3f239 into JuliaHubOSS:master Jan 18, 2024
3 checks passed
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