calyxir / calyx

Intermediate Language (IL) for Hardware Accelerator Generators
https://calyxir.org
MIT License
465 stars 47 forks source link

Calyx website revamp wishlist #1926

Open rachitnigam opened 5 months ago

rachitnigam commented 5 months ago

Tracker for changes to the Calyx website:

YJDoc2 commented 4 months ago

Hey, wrto Fix the playground : When I go to the playground , and click compile on the default-selected options (Sequence example with top down compile control) , there is an error as below

Error:
import not supported in the web demo

I tried two other examples : Conditional and Loops, but both give same error. Is it due to the CI failing as you mentioned, or is it something else? And the CI link the the linked issue no longer exists, is there a newer version of failing CI?

Thanks :)

sampsyo commented 4 months ago

The summary, @YJDoc2, is essentially just bit-rot—yes, we have disabled automatic updates, so it's not getting synced to the most recent version of the Calyx compiler. Getting things working again could be a fun project for anyone interested in getting involved with the Calyx ecosystem. :smiley:

YJDoc2 commented 4 months ago

Hey @sampsyo , thanks for your response! I tried playing around with the code, and got it to run properly on local, but I wanted to ask something regarding the changes I did :

  1. I have changed parcel to vite. parcel v1 is deprecated, and v2 does not have good/any support for loading wasm. I don't think the playground was using any parcel specific stuff apart from loading wasm, so it was a drop-in replacement for npm start , still need to check on the npm build.
  2. There was a bug in code due to which the error I reported was getting shown, so I fixed that. Another thing is, current code would run compile every time we changed compiler pass , which I changed to compile only when compile button is pressed. I observed that with original code, the page would hang up whenever I tried to select/de-select the passes.
  3. After solving the above error, I got a new one IO Error: operation not supported on this platform . After digging, I found that it originates from merge_namespace , which in turn originates from canonicalize_extern->canonicalize . As an experiment, I removed the canonicalize_extern call, and instead passed the original path directly to self.lib.add_extern(abs_path, exts); -> self.lib.add_extern(p.into(), exts); While this made the website work, I don't know if that is correct, or the code generated with it is correct or not. I think the canonicalize function has to do something with the import statements, but not sure. Can you help me with this, and tell me what the intent of this code is?
  4. If I'm correct in above; then as we don't need the import statements in the web (because we already resolve them beforehand) , would it be ok to do a target-based conditional compilation, where for wasm it will do p.into() and for other it will be as it is now?
  5. Also, I personally use npm instead of yarn, so it made a npm lockfile instead of yarn lockfile. Do you have a specific preference for yarn, or is it ok to remove the yarn lockfile and add the npm one instead? This mostly won't matter for local running, but for CI build, it is usually preferable to have a lockfile and do npm/yarn ci so that install is successfuly and does not accidentally update deps with potential to break.

If the above is fine, I'd be happy to clean up the changes I have and make a PR. I also have some other improvements in mind for the page, but can suggest those separately. Let me know. Thanks :)

sampsyo commented 4 months ago

Awesome!!! Thank you so much for looking into this; truly amazing. As you suggest, it would be great to review a PR—your proposed changes all sound excellent, so we can iterate on the details there.

I have changed parcel to vite. parcel v1 is deprecated, and v2 does not have good/any support for loading wasm. I don't think the playground was using any parcel specific stuff apart from loading wasm, so it was a drop-in replacement for npm start , still need to check on the npm build.

Excellent. This is exactly the conclusion I had come to about Parcel v1 & v2. Vite seems like the right alternative to me!

There was a bug in code due to which the error I reported was getting shown, so I fixed that. Another thing is, current code would run compile every time we changed compiler pass , which I changed to compile only when compile button is pressed. I observed that with original code, the page would hang up whenever I tried to select/de-select the passes.

This all sounds fine. If we ever want to get rid of the compile button and just have it automatically recompile, we can explore that separately.

After digging, I found that it originates from merge_namespace , which in turn originates from canonicalize_extern->canonicalize . As an experiment, I removed the canonicalize_extern call, and instead passed the original path directly to self.lib.add_extern(abs_path, exts); -> self.lib.add_extern(p.into(), exts); While this made the website work, I don't know if that is correct, or the code generated with it is correct or not

Got it; this makes sense. I think we probably want to keep the path canonicalization (which helps us avoid duplicating different imports of the same underlying file), but we can make that platform-dependent—that is, when compiling to a wasm target, we'll disable it, because paths are handled differently there.

If I'm correct in above; then as we don't need the import statements in the web (because we already resolve them beforehand) , would it be ok to do a target-based conditional compilation, where for wasm it will do p.into() and for other it will be as it is now?

Yes! This is exactly what I was imagining too.

Also, I personally use npm instead of yarn, so it made a npm lockfile instead of yarn lockfile. Do you have a specific preference for yarn, or is it ok to remove the yarn lockfile and add the npm one instead?

Nope; IMO switching to npm sounds great.