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

Progress on the C backend #671

Merged
merged 75 commits into from
Sep 26, 2024
Merged

Progress on the C backend #671

merged 75 commits into from
Sep 26, 2024

Conversation

AltGr
Copy link
Contributor

@AltGr AltGr commented Aug 30, 2024

Along with many small amelioration / fixes, mainly in the scalc pass and ident renaming processing.

AltGr added 30 commits August 9, 2024 11:47
and it makes things much more complicated, so let's do without for now.
* make Eq an overloaded operator
* no effort to support compound types has been made
* z3 backend impl not done
Including mallocs, this is much cleaner in the resulting code.
It's more tidy, and was required anyway for e.g. division operators
This makes the C runtime interface for date calculations work properly (although
we have placeholders instead of `dates_calc` at the moment)

Includes a few changes:
- use exprs instead of naked_exprs in statements (it's just easier to
  manipulate: passing a naked_expr when an expr is expected is annoying, while
  the opposite is trivial)
- add position to the `Add_dat_dur` operator, which can fail if no specific
  rounding mode is set (in the OCaml and C backends)
- inline closure calls a bit more in `closure_conversion`, for readability
This adds an optional pass that recursively expands equality tests across
structures and enums, on lcalc.

NOTE: this is a temporary solution.

- all tests are completely inlined, which may be a bit bloated
- due to the lack of primitives (and expressive pattern-matching), checking
  equality on enums generates a 2-level pattern matching, quadratic in the
  number of constructors
- this is completely separate from the monomorphisation pass, which morally
  should take care of generating this code (as specific functions rather
  than inlined code)

So, while this should work as a place-holder for now, it actually seems more
reasonable mid-term (before we do it through monomorphisation) to do this
translation at the backend level, i.e. when generating the C code, when we have
full access to the representation of enums.
These should be moved to the lcalc->scalc translation
and remove special handling from the backends (in particular, lifting in the C backend)
Note: we don't really detect the changes, maybe a hash could help; but at least
this avoids an ugly `Index out of bounds` in the case where the changes caused
a line to be shorter than expected.
closure conversion was disabled, but hoisting remained, which was causing
trouble.
This concerns operators like Map, etc.
This solution helps with testing, but may be temporary if we choose to expand
closure application in operator arguments as a part of closure conversion ; it
may be better to let the backends relying on it to handle the closures directly
though.
it now follows a pattern that was generalised
(of course, their output values aren't tested yet for all modules that don't
have asserts; but the testing framework is the next step)
we want our pointers to "const" (in the C sense, i.e. read-only) values in
general; but structures need to be initialised field by field. We handle the
absence of `const` in this case (the pointer target will be made `const` upon
function return).

When we compile with `-O`, though, intermediate variables are removed, and we
have branchings with one side getting an already allocated (`const`) structure,
while the other one will need to init through allocation of individual fields.

In the current state of this patch, we define the structure as non-const and
malloc it when this happens, like in the basic definition case. It works, but in
the branch of the already allocated structure, the malloc is discarded (wasted),
and there is a legitimate warning because we strip the `const` flag from the
source pointer.

I see only two solutions:
- remove all consts (beh, they are very good for understanding what's going on,
  and I'm sure that without them we'll fall into more traps later)
- in the branch(es) needing malloc, allocate and initialise to a temporary
  non-const structure, then affect it to the return pointer ("sealing" it as
  const after initialisation). This basically what happens without `-O` because
  we have more intermediate variables... But we want to target that specifically
  at cases that need malloc.

A variant of the 2nd option could be to define initialiser functions for all
structs, but we loose in readability since the arguments would be unlabelled.
The cleaner way to do it would probably be at the scalc level, depending on the
`--no-struct-literals` flag.
This happens when there are two branches that are bound to an existing struct,
and to a literal one that needs a malloc+non-const;

the fix refines the handling of `no_struct_literals` in scalc, to ensure that
`SLocalInit` is used on a local temporary variable for the constructs needing
malloc.
Also includes some fixes to the propagation of visibility info of types.
now all artefacts related to the module Foo are capitalised, like the module
name.
@AltGr
Copy link
Contributor Author

AltGr commented Sep 24, 2024

Don't forget to check out the changes in related PRs CatalaLang/catala-examples#14 and CatalaLang/french-law#3 — and merge in sync

Copy link
Contributor

@denismerigoux denismerigoux left a comment

Choose a reason for hiding this comment

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

Thank you @AltGr !

@@ -15,10 +15,17 @@
(python/** with_prefix runtime_python)))
(section lib))

; Rescript runtime
;; ; Rescript runtime -- disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disabled ? Should we reenable it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, we could remove the whole directory; the only difference is on the visibility of the thing I guess.
In any case, probably better left for a specific PR.

@AltGr AltGr merged commit 0f409dd into master Sep 26, 2024
5 checks passed
@AltGr AltGr deleted the crun branch September 26, 2024 08:00
@denismerigoux
Copy link
Contributor

Fixes #204

@denismerigoux denismerigoux linked an issue Oct 2, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

C backend for Catala
2 participants