- User Since
- Jan 7 2013, 9:35 AM (366 w, 4 d)
This was (re)landed as rGff171acf8
Oops, this is just the diff from D71681 (which was committed as rG3a05c3969c18 and reverted), but not the whole patch.
I'll reland the whole thing together, but if @yurydelendik or @sunfish have comments I'm happy to hear them.
Thu, Jan 16
Wed, Jan 15
- Cleanup scope, update lld debuginfo test
Thanks, I'll commit this soon.
Mon, Jan 13
- Fix NVPTX
- suppress build warning
Thu, Jan 9
Wed, Jan 8
This is ready for review now, please take a look.
- add doxygen comment
- finish update multi-return
- remove debug prints
- clean up, add comments
Tue, Jan 7
- update tests, add new test for dwarf local var
- start update multi-return test
Fri, Dec 20
- use getDwarfFrameBase on WebAssemblyFrameLowering
- Don't optimize live intervale for frame reg. use global instead of local when not needsSP
- Update NVPTX
(also, the langref says that the order within a priority is unspecfied. is something depending on this ordering (I assume so, since you bothered to change it)?
the commit description says "not ordered to group destructors with common associated objects" but it looks this patch is doing grouping by associated object. Is that intentional?
Also, maybe there should be a test which has destructors of different priorities associated with the same object?
Thu, Dec 19
After thinking a bit more, I'd like to rename DwarfExpression::addTargetIndexLocation to DwarfExpression::addWasmLocation because basically all of the methods on DarfExpression correspond to DW_OP_foo operands, so a method for adding DW_OP_wasm_location should probably just be called addWasmLocation. If we want to later have a utility function to extract some other functionality like converting a TargetIndex, we can always do that in the future when we have another caller.
With that suggestion, this change LGTM
Dec 18 2019
I think that this approach to tracking the frame register through VReg and local substitution will work fairly reliably for now, because the frame register is live through the whole function and is unlikely to be coalesced with other locals or stackified.
One thing I don't like about D69807 is that getFrameRegister is called in a variety of places during generation of various bits of the code as well as in DwarfCompileUnit, whereas getFrameBaseLocation only has this particular meaning and used here (while in the other places getFrameRegister is returning phsyical registers that are meaningless partway through the pipeline.
Dec 17 2019
- Split header generation into separate function
Dec 16 2019
- Merge branch 'objcopy-wasm' into objcopy-add-remove
- Make Object own the OwnedContents instead of Section
- Simplify Section to a struct
- Remove OwnedContents for now, and give Writer::SectionHeader its correct name
Dec 12 2019
Thanks for all your input!
Who else needs to review this? Can you say explicitly, so we can keep this from becoming one of those reviews that hangs in limbo for a long time without any clear path forward?
Dec 11 2019
- review comments
- Review comments 3
Dec 9 2019
Dec 6 2019
Dec 5 2019
I believe this patch is now also ready, please take a look.
- fix newline
- Split tests
I do find it odd that there is a PATH fallback in the existing code in the first place. I agree that basically no compiler other than the "system" compiler should ever use it (and also even the concept of the "system" compiler really only makes much sense on systems like Linux and BSDs where compiling things for the local system is common). I guess the other option here would be to just require that wasm-opt be in the same directory as clang, which we can arrange in wasi-sdk or wherever.
I believe I've addressed all the feedback now.
- Split Reader/Writer into separate files
Dec 4 2019
- Review 2
Dec 3 2019
fix dir name
- rename Wasm directories to wasm
Do you mean wasm instead of Wasm?
- Remove add/remove/dump section support
- Add test that removed section is gone
- Address round 1 of review comments
Thanks for the detailed review. I just uploaded a diff to address the comments (other than splitting the patch apart).
Dec 2 2019
- Add test that removed section is gone
Ping. The target-independent portion of this patch is very small now.
Nov 25 2019
Nov 21 2019
Also if at some point we are able to remove a bunch of the driver logic from emcc and move it here, (e.g. running clang to link instead of lld directly) we'll need to watch out for this.
WRT the LTO directory name, there's theoretically the danger that someone (e.g. emscripten) could be doing a rolling release of the compiler and get invalidated within a major revision. But in that case, 1) they should be rebuilding their libraries on each release anyway, and 2) last time I checked, policy was to make auto-upgrade of bitcode work within a revision. So it's probably not a problem.
Ping. Any other opinions? Perhaps @dblaikie ?
Nov 20 2019
LGTM on the approach, just one more question on the wasm-opt flags.
If the SDK is distributed with the compiler and version-locked, it seems like it should be ok.