RFC/WIP: Try to run gcroot lowering after some LLVM optimizations #17363

yuyichao · 2016-07-10T22:46:56Z

I believe some constant propagation (mostly store to load forwarding) would help some GC frame optimization to identify possible duplicate roots. Instead of making our own for special cases, it would be nice if we could let llvm do that. This requires running some LLVM passes before we do the GC frame lowering.

This PR solves the easy part of it, i.e. making the lowering a true LLVM pass and making sure we have enough information in the optimized IR for liveness analysis. (mostly to make sure that the julia.gc_kill won't be deleted due to dead branch).

The goal is to at least be able to run the lowering after mem2reg (and the instcombine after it). A lot of issues still need to be solve.

We need to trace back bitcast and gep when identifying the address of gcframe stores/loads
Need to make sure we can still identify the use of the value after llvm optimizes the IR (possibly by adding more explicit marks in the IR)

If this approach looks good and tests passes, we can probably merge this first and improve the algorithm later.

Based on #17352 to reduce conflicts.

c.c. @vtjnash

tkelman · 2016-07-10T23:20:17Z

src/ccall.cpp

    }
+    // Mark GC use before **and** after the llvmcall to make sure the arguments
+    // are alive during the llvmcall even if the llvmcall has `unreachable`.
+    // If the llvmcall generates GC safepoint, it might need to emit it's own


tkelman · 2016-07-11T01:38:56Z

src/llvm-gcroot.cpp

+    typedef unsigned id;
+    enum {
+        // an assignment to a gcroot exists in the basic-block
+        // (potentially no live-in from the predecessor basic-blocks)


what does "live-in" mean?

The value in the slot when getting into this basic block is live at that point.

could that be explained in a handful more words? having confusing jargon in the comments sort of defeats the explanatory purpose of them

I was just reading some doc about liveness analysis and I've seen a few use of it.

a reference link would work too

JeffBezanson · 2016-07-11T03:16:15Z

Does this fix #17342?

yuyichao · 2016-07-11T03:38:16Z

Does this fix #17342

Unfortunately no (This PR is only about code rearrangement and fixing incompatibility rather than improving the optimizations). With the load forwarding done before the gc frame lowering, it should be much easier to fix that issue though....

vtjnash · 2016-07-11T14:53:39Z

src/jitlayers.cpp

@@ -52,6 +54,9 @@ static void addOptimizationPasses(T *PM)
 #ifndef INSTCOMBINE_BUG
    PM->add(createInstructionCombiningPass()); // Cleanup for scalarrepl.
 #endif
+    // Let the InstCombine pass remove the unnecessary load of
+    // safepoint address first
+    PM->add(createLowerPTLSPass(imaging_mode, tbaa_const));


this is a module pass, but addOptimizationPasses was templated to accept the FunctionPassManager too. I think we only need to make module pass managers now (which might also make the LowerGCFramePass easier, since it can walk the uses list instead of walking the Function Instructions), so it might be good to update the function signature.

fixed upstream

tkelman · 2016-07-16T12:30:16Z

src/llvm-ptls.cpp

+
+// This file defines two entry points:
+//     global function annotateSimdLoop: mark a loop as a SIMD loop.
+//     createLowerSimdLoopPass: construct LLVM for lowering a marked loop later.


these comments don't seem accurate

vtjnash · 2016-07-18T17:45:02Z

src/llvm-ptls.cpp

+    // DLLImport only needs to be set for the shadow module
+    // it just gets annoying in the JIT
+    proto->setDLLStorageClass(GlobalValue::DefaultStorageClass);
+#endif


this can go away (it isn't needed unless you call addComdat)

but if we bring addComdat back we'd need to add this back?

no, it's permanently gone. besides, we don't need comdat on variables or dllimports.

but that does bring up the point that this declaration will be wrong for a no-threads build on windows, since at the end of the pass, it'll be missing the DLLImport attribute (usually would have been fixed up by the merge-module pass).

So what needs to be fixed here? I have no idea how DLLImport works on windows....

You just have to mark AoT which symbols will be statically linked into the binary and which will be found via the dynamic linker (dllimport)

vtjnash · 2016-07-18T17:46:45Z

lgtm. can you rebase & update, to make sure there aren't logical merge conflicts with the codegen / jitlayers split, & merge.

yuyichao · 2016-07-19T13:14:45Z

src/llvm-ptls.cpp

+        proto->setDLLStorageClass(GlobalValue::DLLImportStorageClass);
+#else
+        proto->setLinkage(GlobalValue::DLLImportLinkage);
+#endif


@vtjnash like this?

* Make the pass more robust against dead code elimination * Remove module level setup code (so that the the GC frame lowering can be a `FunctionPass`) * Create PTLS lowering (module) pass since it needs to be run after the GC frame lowering * Set DLLImport attribute for `jl_tls_states` on windows, since it is not handled by module merger anymore

…#17363.

yuyichao added compiler:codegen Generation of LLVM IR and native code GC Garbage collector labels Jul 10, 2016

yuyichao force-pushed the yyc/codegen/gc-pass branch 2 times, most recently from dad001b to 1455659 Compare July 10, 2016 23:02

tkelman reviewed Jul 10, 2016
View reviewed changes

yuyichao force-pushed the yyc/codegen/gc-pass branch from 1455659 to 8e4a732 Compare July 11, 2016 00:05

tkelman reviewed Jul 11, 2016
View reviewed changes

vtjnash reviewed Jul 11, 2016
View reviewed changes

yuyichao force-pushed the yyc/codegen/gc-pass branch 4 times, most recently from 8a53e5f to 1c114f5 Compare July 16, 2016 01:35

tkelman reviewed Jul 16, 2016
View reviewed changes

yuyichao mentioned this pull request Jul 16, 2016

Fix non-threading compilation #17453

Closed

vtjnash reviewed Jul 18, 2016
View reviewed changes

yuyichao force-pushed the yyc/codegen/gc-pass branch 2 times, most recently from 265e955 to 2e26033 Compare July 19, 2016 13:14

yuyichao reviewed Jul 19, 2016
View reviewed changes

yuyichao force-pushed the yyc/codegen/gc-pass branch from 2e26033 to 4a8932b Compare July 20, 2016 04:05

yuyichao added 3 commits July 21, 2016 08:03

Make the gc uses mark robust against unreachable and noreturn functions.

c8f1b62

Make GC frame lowering a LLVM pass

c9b5d9d

yuyichao force-pushed the yyc/codegen/gc-pass branch from 4a8932b to c9b5d9d Compare July 21, 2016 12:04

vtjnash merged commit 1fcb81c into master Jul 21, 2016

vtjnash deleted the yyc/codegen/gc-pass branch July 21, 2016 19:16

maleadt added a commit to JuliaGPU/CUDAdrv.jl that referenced this pull request Jul 22, 2016

Remove the workaround for JuliaLang/julia#17288 after JuliaLang/julia…

3aa4dc0

…#17363.

Neumann-A mentioned this pull request May 31, 2023

Differentiate between Windows and MinGW #49867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC/WIP: Try to run gcroot lowering after some LLVM optimizations #17363

RFC/WIP: Try to run gcroot lowering after some LLVM optimizations #17363

yuyichao commented Jul 10, 2016

tkelman Jul 10, 2016

tkelman Jul 11, 2016

yuyichao Jul 11, 2016

tkelman Jul 11, 2016

yuyichao Jul 11, 2016

tkelman Jul 11, 2016

JeffBezanson commented Jul 11, 2016

yuyichao commented Jul 11, 2016

vtjnash Jul 11, 2016 •

edited

Loading

tkelman Jul 16, 2016

vtjnash Jul 18, 2016

tkelman Jul 18, 2016

vtjnash Jul 18, 2016 •

edited

Loading

yuyichao Jul 18, 2016

vtjnash Jul 19, 2016

vtjnash commented Jul 18, 2016

yuyichao Jul 19, 2016

vtjnash Jul 19, 2016

RFC/WIP: Try to run gcroot lowering after some LLVM optimizations #17363

RFC/WIP: Try to run gcroot lowering after some LLVM optimizations #17363

Conversation

yuyichao commented Jul 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jul 11, 2016

yuyichao commented Jul 11, 2016

vtjnash Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Jul 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash Jul 11, 2016 •

edited

Loading

vtjnash Jul 18, 2016 •

edited

Loading