Externalize builtins and fix bugs #637
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 87.78% 87.94% +0.15%
==========================================
Files 84 86 +2
Lines 20721 20903 +182
==========================================
+ Hits 18190 18383 +193
+ Misses 2531 2520 -11 ☔ View full report in Codecov by Sentry. |
yamaguchi1024
changed the title
src/exo/LoopIR_compiler.py
Outdated
| @@ -434,17 +434,17 @@ def from_lines(x): | |||
| // Compiler feature macros adapted from Hedley (public domain) | |||
| // https://github.com/nemequ/hedley | |||
|
|
|||
| #if defined(__has_builtin) | |||
There was a problem hiding this comment.
I am confused. Why were these changed? These are referring to C builtins, not Exo.
src/exo/LoopIR_compiler.py
Outdated
| private_fwd_decls = [] | ||
| proc_bodies = [] | ||
| instrs_global = [] | ||
| new_proc_list = [] |
There was a problem hiding this comment.
I found this name confusing. Maybe you can call it non_instr_procs. Or at least add a comment after it.
There was a problem hiding this comment.
analyzed_proc_list ?
|
I think a more ergonomic way of implementing this feature would be something like the following: This would effectively encode all the information you are encoding in the class and it is more akin to how we externalized instructions definitions. There is the issue however of how to make sure these externs' arguments are dynamically typed which was the case before. We might want to allow these externs to be overloaded to avoid having to write on per-type or maybe this is not an issue because it is the same problem for instruction. We might though need to allow them to be rewritten modulo precisions. e.g. |
|
I like this interface!! It's like we're adding Exo type annotation (e.g., f32) to Python function, but the |
|
@SamirDroubi I think |
|
Flagging here that we don't have documentation for builtins/externs. Maybe worth fixing that in this PR |
- Rename builtins to externs - Externalize externs - Fix bug 1: extern did not work on read expressions with index accesses - Fix bug 2: extern fnarg codegen should be comp_e not comp_fnarg - Fix bug 3: extern did not work on constants - Change pattern_match and parse_fragment to take local and global variables in scope.
yamaguchi1024
requested review from
akeley98
and removed request for
skeqiqevian and
SamirDroubi
There was a problem hiding this comment.
I tried to scan through all the nontrivially changed files and the grammar changes and it looks reasonable so far, except the one performance note I made. I don't understand what call_depth is doing very well ... this is something about being able to look-up Python local variables?
| @@ -166,15 +172,18 @@ def __init__( | |||
| self.is_fragment = is_fragment | |||
|
|
|||
| self.push() | |||
| special_cases = ["stride"] | |||
There was a problem hiding this comment.
Creating this special_cases list and then searching it linearly (in the s.value.func.id in special_cases) looks like potentially expensive "accumulated cost" considering how many times this parser is used, e.g., to parse all procs and all patterns in @instr. Can this be an is_special_case lambda that just searches for the func id in the 2 namespaces (+ the special stride case)?
There was a problem hiding this comment.
Something like
def is_special_case(func_id):
return (
func_id == "stride"
or isinstance(self.globals.get(func_id), Extern)
or isinstance(self.locals.get(func_id), Extern)
)
...
if len(module_ast) == 1:
s = module_ast[0]
if isinstance(s, pyast.Expr) and (
not isinstance(s.value, pyast.Call)
or is_special_case(s.value.func.id)
):
is_expr = True
There was a problem hiding this comment.
i think special_cases is only used once which is to check patterns that are of the form <extern>(...) so I don't think there is a big cost to doing this
There was a problem hiding this comment.
It's possible that scanning all the global and local variables (at the beginning of Parser) is expensive, but looking up func_id each time in globals and locals is also expensive, so 🤷♀️
There was a problem hiding this comment.
I think this is fine
yeah, when the user calls the api to create a pattern, the pattern might reference externs in the local scope or global scope where the user did the call. the api might make additional calls to other api functions, which then need to figure out the local scope of the function that originally called into the api. so the api needs to keep track of how many calls were made after the user initially called into the api |
| args = [] | ||
| for a in e.args: | ||
| if a.type == T.R: | ||
| args.append(self.coerce_e(a, btyp)) |
There was a problem hiding this comment.
are externs expected to support every available precision on Exo?
There was a problem hiding this comment.
Yes! But this is coercing the argument when it was T.R (which means the precision was not specified during scheduling)
| @@ -166,15 +172,18 @@ def __init__( | |||
| self.is_fragment = is_fragment | |||
|
|
|||
| self.push() | |||
| special_cases = ["stride"] | |||
There was a problem hiding this comment.
i think special_cases is only used once which is to check patterns that are of the form <extern>(...) so I don't think there is a big cost to doing this
comp_enotcomp_fnargFixes #505