JuliaHubOSS / llvm-cbe

resurrected LLVM "C Backend", with improvements
Other
838 stars 144 forks source link

support llvm 17 #187

Closed 5c4lar closed 8 months ago

vtjnash commented 8 months ago

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

5c4lar commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Seems like more clang-format fixes are still needed?

5c4lar commented 8 months ago

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 commented 8 months ago

Looks like it used to be disabled in the distant past: https://github.com/JuliaHubOSS/llvm-cbe/blob/2ca604e464610df1020975fcf22d2b9f2d3026f6/tools/llvm-cbe/llvm-cbe.cpp#L306-L314

I don't know what might be wrong about the current output: https://github.com/JuliaHubOSS/llvm-cbe/blob/b1efd16563bbb633705494ed979cdb7cac8084d3/lib/Target/CBackend/CBackend.cpp#L3878-L3880

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 commented 8 months ago

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 commented 8 months ago

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 :)