yamt wrote:
> I'm not sure about the common practice in this repo, but Github in general
> supports merge workflows much better. Especially in a long-running large PRs,
> if you force-push, it's hard for the reviewers to know what has changed since
> their last review, because all the history
sbc100 wrote:
Is this good to land now? @yamt do you have commit access or should @aheejin
or myself land this?
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/aheejin approved this pull request.
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aheejin wrote:
I'm not sure about the common practice in this repo, but Github in general
supports merge workflows much better. Especially in a long-running large PRs,
if you force-push, it's hard for the reviewers to know what has changed since
their last review, because all the history his
yamt wrote:
> @yamt Given that this PR is not large and we already went through it anyway,
> nevermind what I said about rebasing, and please use whatever method you are
> convenient with. But just FYI you can do `git merge` to avoid rebasing (I'm
> not asking you to do it here; just in case
aheejin wrote:
@yamt Given that this PR is not large and we already went through it anyway,
nevermind what I said about rebasing, and please use whatever method you are
convenient with. But just FYI you can do `git merge` to avoid rebasing (I'm
not asking you to do it here; just in case you
yamt wrote:
@aheejin may i rebase this PR as it now has conflicts?
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yamt wrote:
@aheejin i updated the rest of comments
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aheejin edited
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aheejin edited
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -129,27 +129,21 @@
///
/// If there are calls to setjmp()
///
-/// 2) In the function entry that calls setjmp, initialize setjmpTable and
-///sejmpTableSize as follows:
-/// setjmpTableSize = 4;
-/// setjmpTable = (int *) malloc(40);
-/// setjmpTable[0] =
@@ -1268,42 +1259,21 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
LLVMContext = F.getContext();
IRBuilder<> IRB(C);
SmallVector ToErase;
- // Vector of %setjmpTable values
- SmallVector SetjmpTableInsts;
- // Vector of %setjmpTableSize
https://github.com/aheejin commented:
Nice simplification! Thank you for working on this.
Some minor nits about comments:
- You removed
https://github.com/llvm/llvm-project/blob/43fc921795bd130a325c013d60f209b5c6128fc7/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L213-L215
@@ -129,27 +129,21 @@
///
/// If there are calls to setjmp()
///
-/// 2) In the function entry that calls setjmp, initialize setjmpTable and
-///sejmpTableSize as follows:
-/// setjmpTableSize = 4;
-/// setjmpTable = (int *) malloc(40);
-/// setjmpTable[0] =
yamt wrote:
> i tried to run the relevant emscripten tests. but i have not succeeded yet
> even w/o these changes. (i'm an emscripten newbie.)
they passed with the latest version of this PR and the emscripten PR.
https://github.com/llvm/llvm-project/pull/84137
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 6c1bfa0aa292d81102a9aeb86ca4264516beb634 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 01/26] [WebAssembly] Implement an alternative translation for
https://github.com/yamt edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yamt wrote:
> If possible, can you not force-push? It's hard to track changes between
> commits with force-pushes. Thank you!
i had to rebase for some unrelated reasons. i will avoid it next time.
https://github.com/llvm/llvm-project/pull/84137
___
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
https://github.com/aheejin edited
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aheejin commented:
If possible, can you not force-push? It's hard to track changes between commits
with force-pushes. Thank you!
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -999,25 +1017,43 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module ) {
// Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
FunctionType *FTy = FunctionType::get(
IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-
@@ -1738,10 +1792,16 @@ void
WebAssemblyLowerEmscriptenEHSjLj::handleLongjmpableCallsForWasmSjLj(
BasicBlock *ThenBB = BasicBlock::Create(C, "if.then", );
BasicBlock *EndBB = BasicBlock::Create(C, "if.end", );
Value *EnvP = IRB.CreateBitCast(Env, getAddrPtrType(),
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
https://github.com/sbc100 commented:
Let me know if you want some help testing alsongside the emscripten side.
Alternatively myself or @aheejin could perform the emscripten tests.
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits
https://github.com/sbc100 edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -198,9 +198,18 @@
///
/// If there are calls to setjmp()
///
-/// 2) and 3): The same as 2) and 3) in Emscripten SjLj.
-/// (setjmpTable/setjmpTableSize initialization + setjmp callsite
-/// transformation)
+/// 2) In the function entry that calls setjmp, initialize
+///
yamt wrote:
@aheejin @sbc100
i think i resolved most of your feedback in this PR and the emscripten PR.
i tried to run the relevant emscripten tests. but i have not succeeded yet even
w/o these changes. (i'm an emscripten newbie.)
https://github.com/llvm/llvm-project/pull/84137
https://github.com/yamt ready_for_review
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 6c1bfa0aa292d81102a9aeb86ca4264516beb634 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 01/20] [WebAssembly] Implement an alternative translation for
https://github.com/yamt edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 6c1bfa0aa292d81102a9aeb86ca4264516beb634 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 01/19] [WebAssembly] Implement an alternative translation for
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 2dbaf265255a5fa9643a8092ec2dffa881d2cf93
1b561bf2d7e36b043e24d50eda612bbb12f454ef --
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 6c1bfa0aa292d81102a9aeb86ca4264516beb634 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 01/17] [WebAssembly] Implement an alternative translation for
https://github.com/yamt edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt converted_to_draft
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 6c1bfa0aa292d81102a9aeb86ca4264516beb634 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 01/13] [WebAssembly] Implement an alternative translation for
sbc100 wrote:
> @aheejin @sbc100 let me confirm the plan on this PR. i can remove the option
> `-mllvm -experimental-wasm-enable-alt-sjlj` by making it unconditionally
> true, and update tests, right?
That is my understanding yes.
https://github.com/llvm/llvm-project/pull/84137
yamt wrote:
@aheejin @sbc100
let me confirm the plan on this PR.
i can remove the option `-mllvm -experimental-wasm-enable-alt-sjlj` by making
it unconditionally true, and update tests, right?
https://github.com/llvm/llvm-project/pull/84137
___
aheejin wrote:
> > > Given that we don't need `setjmpTableSize` anymore and `setjmpTable`
> > > doesn't change, we don't need the whole block here from line 1463 ~ line
> > > 1503 doing SSA updates anymore:
> >
> >
> > i get errors like the following if i simply put the SSA update things in
aheejin wrote:
Had a chance to chat with @sbc100 offline and we agreed we can probably do this:
> 1. Add library functions in emscripten
> 2. Replace the current logic in LLVM with new code. (Without `if`~`else`).
> Tests can be updated with this.
> 3. Remove old library functions in
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 1283ae6b5536810f8fbe183eda80997aa9f5cdc3 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 1/7] [WebAssembly] Implement an alternative translation for
yamt wrote:
> > Given that we don't need `setjmpTableSize` anymore and `setjmpTable`
> > doesn't change, we don't need the whole block here from line 1463 ~ line
> > 1503 doing SSA updates anymore:
>
> i get errors like the following if i simply put the SSA update things in
>
yamt wrote:
> Given that we don't need `setjmpTableSize` anymore and `setjmpTable` doesn't
> change, we don't need the whole block here from line 1463 ~ line 1503 doing
> SSA updates anymore:
i get errors like the following if i simply put the SSA update things in
`!EnableWasmAltSjLj` block.
yamt wrote:
> @yamt, do you want to submit a PR to emscripten adding new library functions?
> I can do that too, but given that this is your code, if you want to do it
> it'd be good. About where to add them... to make the transition smooth,
>
yamt wrote:
> > In terms of getting this landed and tested, I wonder which path we should
> > take:
> >
> > 1. Land this now, without tests, then update emscripten then come back and
> > flip the default, at which point the existing tests will get updated.
> > 2. Duplicate/update the the
@@ -1738,10 +1792,16 @@ void
WebAssemblyLowerEmscriptenEHSjLj::handleLongjmpableCallsForWasmSjLj(
BasicBlock *ThenBB = BasicBlock::Create(C, "if.then", );
BasicBlock *EndBB = BasicBlock::Create(C, "if.end", );
Value *EnvP = IRB.CreateBitCast(Env, getAddrPtrType(),
@@ -999,25 +1017,43 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module ) {
// Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
FunctionType *FTy = FunctionType::get(
IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
aheejin wrote:
> > > In terms of getting this landed and tested, I wonder which path we should
> > > take:
> > >
> > > 1. Land this now, without tests, then update emscripten then come back
> > > and flip the default, at which point the existing tests will get updated.
> > > 2.
sbc100 wrote:
> > In terms of getting this landed and tested, I wonder which path we should
> > take:
> >
> > 1. Land this now, without tests, then update emscripten then come back and
> > flip the default, at which point the existing tests will get updated.
> > 2. Duplicate/update the the
https://github.com/aheejin edited
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aheejin wrote:
> In terms of getting this landed and tested, I wonder which path we should
> take:
>
> 1. Land this now, without tests, then update emscripten then come back and
> flip the default, at which point the existing tests will get updated.
> 2. Duplicate/update the the existing
@@ -999,25 +1017,43 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module ) {
// Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
FunctionType *FTy = FunctionType::get(
IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -1291,19 +1327,29 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runSjLjOnFunction(Function ) {
Type *IntPtrTy = getAddrIntType();
Constant *size = ConstantInt::get(IntPtrTy, 40);
IRB.SetInsertPoint(SetjmpTableSize);
- auto *SetjmpTable = IRB.CreateMalloc(IntPtrTy,
@@ -54,6 +54,9 @@ cl::opt
// setjmp/longjmp handling using wasm EH instrutions
cl::opt WebAssembly::WasmEnableSjLj(
"wasm-enable-sjlj", cl::desc("WebAssembly setjmp/longjmp handling"));
+cl::opt WebAssembly::WasmEnableAltSjLj(
+"experimental-wasm-enable-alt-sjlj",
+
@@ -1738,10 +1792,16 @@ void
WebAssemblyLowerEmscriptenEHSjLj::handleLongjmpableCallsForWasmSjLj(
BasicBlock *ThenBB = BasicBlock::Create(C, "if.then", );
BasicBlock *EndBB = BasicBlock::Create(C, "if.end", );
Value *EnvP = IRB.CreateBitCast(Env, getAddrPtrType(),
@@ -386,6 +386,20 @@ void WebAssembly::addClangTargetOptions(const ArgList
,
// Backend needs '-exception-model=wasm' to use Wasm EH instructions
CC1Args.push_back("-exception-model=wasm");
}
+
+if (Opt.starts_with("-experimental-wasm-enable-alt-sjlj")) {
@@ -300,6 +315,7 @@ class WebAssemblyLowerEmscriptenEHSjLj final : public
ModulePass {
bool EnableEmEH; // Enable Emscripten exception handling
bool EnableEmSjLj; // Enable Emscripten setjmp/longjmp handling
bool EnableWasmSjLj; // Enable Wasm setjmp/longjmp
https://github.com/aheejin commented:
Nice work! Sorry for the late reply. I thought we needed the table because we
need to distinguish two different calls from the same function, for example, if
`foo` calls `setjmp` and also recursively calls `foo` again, so the same
callsite can have two
https://github.com/aheejin edited
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/sbc100 commented:
In terms of getting this landed and tested, I wonder which path we should take:
1. Land this now, without tests, then update emscripten then come back and
flip the default, at which point the existing tests will get updated.
2. Duplicate/update the the
@@ -999,25 +1002,42 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module ) {
// Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
FunctionType *FTy = FunctionType::get(
IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-
https://github.com/yamt updated https://github.com/llvm/llvm-project/pull/84137
>From 1283ae6b5536810f8fbe183eda80997aa9f5cdc3 Mon Sep 17 00:00:00 2001
From: YAMAMOTO Takashi
Date: Fri, 9 Feb 2024 15:49:55 +0900
Subject: [PATCH 1/3] [WebAssembly] Implement an alternative translation for
@@ -999,25 +1002,42 @@ bool
WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module ) {
// Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
FunctionType *FTy = FunctionType::get(
IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-
https://github.com/sbc100 edited https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/sbc100 commented:
Thanks for working on this!
I'm excited to see this land and become less emscripten-specific.
I'm hoping that once we get the emscripten side landed we can remove the new
option very soon too.
https://github.com/llvm/llvm-project/pull/84137
aheejin wrote:
Can I take a look before landing this? I'm OOO this afternoon but will be back
tomorrow.
https://github.com/llvm/llvm-project/pull/84137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: YAMAMOTO Takashi (yamt)
Changes
Instead of maintaining per-function-invocation malloc()'ed tables to track
which functions each label belongs to, store the equivalent info in jump
buffers (jmp_buf) themselves.
Also, use a less
https://github.com/yamt created https://github.com/llvm/llvm-project/pull/84137
Instead of maintaining per-function-invocation malloc()'ed tables to track
which functions each label belongs to, store the equivalent info in jump
buffers (jmp_buf) themselves.
Also, use a less emscripten-looking
81 matches
Mail list logo
