ndstrides: Allow multi-file IRRT + Exceptions in IRRT #508
No reviewers
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#508
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ndstrides-enhance-irrt"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
-DIRRT_DEBUG_ASSERT
in nac3core/build.rs. Debug assertions throw exceptions..clang-format
for reformatting.@ -0,0 +1,3 @@
BasedOnStyle: Microsoft
Why Microsoft style? Wouldn't this introduce inconsistencies with
demo.c
where opening braces are on the same line as the statement?I picked Microsoft arbitrarily. Could we reformat
demo.c
to be Microsoft style too?I personally use LLVM, as Microsoft's opening brace style makes the source code unnecessarily bloated.
Sure I can reformat everything to LLVM, should take little work. Later commits too.
@ -33,6 +33,7 @@ use inkwell::{
OptimizationLevel,
};
use itertools::Itertools;
use nac3core::codegen::irrt::setup_irrt_exceptions;
Fold this into
use nac3core::codegen::{...}
.@ -0,0 +1,4 @@
#include <irrt/exception.hpp>
#include <irrt/int_types.hpp>
#include <irrt/math_util.hpp>
#include <irrt/original.hpp>
Why is this necessary? Why not have multiple IRRT C++ files and link the LLVM bitcode at build-time?
Sure, but I didn't want to deal with having to managing temporary files. I can implement that now.
Update: I think implementing it would make the process a bit too complicated, at least with the solution I came up with.
build.rs
domake
and pass in all the necessary configurations as environment variables.*.cpp
files recursively and compile each of them, and somehow save them temporarily in a directory.build.rs
reads the aggregate and filter the IR with regex.irrt.o
.If you have to do this ugly Makefile trick then let's just keep it the way it is for now.
@ -0,0 +4,4 @@
template <typename SizeT> struct CSlice
{
uint8_t *base;
*
on the left, i.e.uint8_t*
.Forced by Microsoft
Still not reformatted.
@ -0,0 +7,4 @@
/**
* @brief Implementation of `strlen()`.
*/
uint32_t length(const char *str)
Remove this and use
__builtin_strlen
instead.@ -0,0 +10,4 @@
#define raise_debug_assert(SizeT, msg, param1, param2, param3) \
raise_exception(SizeT, EXN_ASSERTION_ERROR, "IRRT debug assert failed: " msg, param1, param2, param3);
#define debug_assert_eq(SizeT, lhs, rhs) \
Why not just
IRRT_DEBUG_ASSERT_BOOL
itself will be used to write stuff like:I could place a bunch of
#ifdef IRRT_DEBUG_ASSERT
blocks but I find it ugly, though I am not sure what modern C++ people do nowadays.You can keep the
_BOOL
variable.I would say I would do it as
But I suggested the original version as it most resembles how
assert
in C is implemented in the<assert.h>
header.@ -0,0 +45,4 @@
int64_t params[3];
};
const int64_t NO_PARAM = 0;
constexpr
I would like to understand why.
I am not too familiar with
constexpr
and why people put them everywhere.constexpr
indicates that this is to be a compile-time constant, and impliesinline
. This ensures that the variable does not escape this compilation unit and prevents ODR violations. AFAIK these variables also do not show up in the compiled ELF file.https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#con5-use-constexpr-for-values-that-can-be-computed-at-compile-time
@ -0,0 +53,4 @@
{
Exception<SizeT> e = {
.id = id,
.filename = {.base = (uint8_t *)filename, .len = (int32_t)cstr::length(filename)},
Add spaces between casts (or preferably, use
reinterpret_cast
).Microsoft again.
I am not using
reinterpret_cast
because it is convenient. Do they make a difference? or should I usereinterpret_cast
for consistency.They are functionally the same, but you should use modern C++ regardless.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast
Ah okay
@ -0,0 +57,4 @@
.line = line,
.column = 0,
.function = {.base = (uint8_t *)function, .len = (int32_t)cstr::length(function)},
.msg = {.base = (uint8_t *)msg, .len = (int32_t)cstr::length(msg)},
Same here.
Microsoft.
@ -0,0 +62,4 @@
e.params[0] = param0;
e.params[1] = param1;
e.params[2] = param2;
__nac3_raise((void *)&e);
Same here.
@ -0,0 +1,372 @@
#pragma once
Probably best to split
original.hpp
into separate files, since you're already doing that.@ -931,1 +931,4 @@
}
/// Initialize all global `EXN_*` exception IDs in IRRT with the [`SymbolResolver`].
pub fn setup_irrt_exceptions<'ctx>(
Couldn't you just set the exceptions while loading the IRRT? Since IRRT depends on the exception specifications anyways.
@ -14,6 +14,7 @@ use inkwell::{
memory_buffer::MemoryBuffer, passes::PassBuilderOptions, support::is_multithreaded, targets::*,
OptimizationLevel,
};
use nac3core::codegen::irrt::setup_irrt_exceptions;
Fold this into
use nac3core::{codegen::{...}, ...}
@ -0,0 +52,4 @@
int64_t param0, int64_t param1, int64_t param2)
{
Exception<SizeT> e = {
.id = id,
Designated initializer syntax requires C++20. Either use the standard syntax or add
-std=c++20
tobuild.rs
.Revised. Squashing commits when everything is ok.
The positions of pointers
*
and references&
are still incorrect, and I still see many uses of C-style casts. Please change those before submitting for re-review.@ -0,0 +11,4 @@
raise_exception(SizeT, EXN_ASSERTION_ERROR, \
"IRRT debug assert failed: " msg, param1, param2, param3);
#define debug_assert_eq(SizeT, lhs, rhs) if constexpr (IRRT_DEBUG_ASSERT_BOOL)
You still need the trailing
\
for newlines, no?This still needs fixing AFAICT.
Do you want me to help you do a quick rebase pass to fix all the formatting issues instead?
I am fully relying on
.clang-format
to reformat, and it really does not likeT*
. Many of the formatting issues come from me just doingblindly.
Perhaps we should settle on new
.clang-format
instead? Like using yours. Since I will have to keep reformatting my future PRs, and doing it manually is definitely not a sane option.Also I can't find any C-style casts in
nac3core/irrt
, or at least withrg '\*\)' nac3core/irrt
and vim....unless you are talking about
nacstandalone/
Switched to your .clang-format and reformatted everything. How see?
ff1088b429
to9ac4c73797
Much better, thanks.
Thanks. You could squash the final few commits and then we can merge it.
b326532099
to048950b7f0
Rebased to clean up the branch