Rebase of julia effects to LLVM #50188

gbaraldi · 2023-06-15T20:24:02Z

What other effects can we do, i.e readnone etc.

gbaraldi · 2023-06-16T01:26:13Z

We might be able to add readnone if we see we have no pointer arguments

pchintalapudi · 2023-06-16T02:41:47Z

That might conflict with the pgcstack being passed as an argument.

wsmoses · 2023-06-16T06:26:54Z

src/codegen.cpp

+        attr.addAttribute("consistent");
+    }
+    if (effect_free == 0) {
+        attr.addAttribute("effect_free");


Is this not desirable to make readnone/only/write only where applicable

Yeah, I think we can add more things to the functions for sure.

wsmoses · 2023-06-16T06:28:45Z

src/codegen.cpp

+        attr.addAttribute("noinbounds");
+    }
+    if (is_decl && consistent == 0 && effect_free == 0 && nothrow == 1 && terminates == 1){
+        attr.addAttribute(Attribute::Speculatable);


You probably want to change these to be added at functionindex

It may be worthwhile considering if any attributes on params are also useful

I think they might be, but I'm not sure what information we can infer from here

Also when creating the AttrList we pass the builder to the FnAttr builder

wsmoses · 2023-06-16T06:29:33Z

src/codegen.cpp

@@ -4204,7 +4256,13 @@ static jl_cgval_t emit_call_specfun_other(jl_codectx_t &ctx, bool is_opaque_clos
        TheCallee = ai.decorateInst(ctx.builder.CreateAlignedLoad(TheCallee->getType(), GV, Align(sizeof(void*))));
    }
    CallInst *call = ctx.builder.CreateCall(cft, TheCallee, argvals);
-    call->setAttributes(returninfo.attrs);
+
+    if (isa<Function>(TheCallee)){


Make this a F=dyn_cast for ease?

gbaraldi · 2023-06-16T13:42:56Z

I guess readnone is always wrong with or without the pgcstack arg, at least it's not a global anymore

pchintalapudi · 2023-06-19T14:25:32Z

One thing that should be possible is that if we determine a function has the terminates effect we should just not insert a safepoint at entry.

gbaraldi · 2023-06-19T14:39:45Z

The issue with that is that if you have a for loop that calls the function a bunch of times you aren't ever hitting a safepoint.

pchintalapudi · 2023-06-19T14:53:18Z

That's true, but if we've proven a function to terminate, its runtime is probably really small and the safepoint will probably take up a good fraction of its runtime, so it's likely that even a loop on a terminates function will itself exit quickly.

gbaraldi · 2023-06-19T16:07:19Z

But I think that's kind of the point, because we don't know termination for loops, only for the function itself, and it is what's guaranteeing that a GC may run.

gbaraldi · 2023-06-26T14:30:59Z

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

nanosoldier · 2023-06-26T22:31:26Z

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

gbaraldi · 2023-06-27T11:56:00Z

So digging into the the failure, it is reproducible.

julia> CHUNK_TYPE = Dict(1=>"FileHeader")
Dict{Int64, String} with 1 entry:
  1 => "FileHeader"

julia> foo(v) = [length(x) for x in values(v)]
foo (generic function with 1 method)

julia> foo(CHUNK_TYPE)

is enough to reproduce.

Interestingly, while debugging with Cthulhu, I trigger

Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   g::Base.Generator{Base.ValueIterator{Dict{Int64, String}}, var"#1#2"}.iter
 • iterate(g::Base.Generator{Base.ValueIterator{Dict{Int64, String}}, var"#1#2"}.iter::Base.ValueIterator{Dict{Int64, String}}, s::Tuple{}...)
   g::Base.Generator{Base.ValueIterator{Dict{Int64, String}}, var"#1#2"}.f
   y::Union{Nothing, Tuple{String, Int64}}[1]
   g::Base.Generator{Base.ValueIterator{Dict{Int64, String}}, var"#1#2"}.f::var"#1#2"(y::Union{Nothing, Tuple{String, Int64}}[1]::String)
   y::Union{Nothing, Tuple{String, Int64}}[2]
   ↩
Assertion failed: (ndt->types == NULL), function jl_reinstantiate_inner_types, file jltypes.c, line 2506.

[24213] signal (6): Abort trap: 6
in expression starting at REPL[5]:1
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 3765944 (Pool: 3760493; Big: 5451); GC: 6
fish: Job 1, './julia' terminated by signal SIGABRT (Abort)

gbaraldi · 2023-06-27T15:30:08Z

So that might have been a red herring but doing

 @code_llvm raw=true dump_module=true foo(CHUNK_TYPE)

shows that we codegen

    %32 = call swiftcc i64 @j_length_234({}*** nonnull swiftself %0, {} addrspace(10)* null), !dbg !134

which doesn't seem very correct.

src/codegen.cpp

gbaraldi · 2023-07-10T21:56:38Z

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

gbaraldi · 2023-07-10T21:59:43Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2023-07-11T05:14:49Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

nanosoldier · 2023-07-11T10:52:57Z

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

vtjnash · 2023-08-08T00:35:11Z

src/codegen.cpp

@@ -4327,7 +4379,12 @@ static jl_cgval_t emit_call_specfun_other(jl_codectx_t &ctx, bool is_opaque_clos
        setName(ctx.emission_context, TheCallee, namep);
    }
    CallInst *call = ctx.builder.CreateCall(cft, TheCallee, argvals);
-    call->setAttributes(returninfo.attrs);
+    if (auto fun = dyn_cast<Function>(TheCallee)){
+        fun->setAttributes(AttributeList::get(ctx.builder.getContext(), {get_attrs_ipoeffects(ctx.builder.getContext(), effects, true, has_ptr_arg), returninfo.attrs, attrs}));


It is generally pretty sketchy to mutate the callee to assume information that was only valid for this particular caller call site. I would delete this change.

The slightly annoying thing is that speculatable can't be passed to call sites, only on declarations. Also aren't the ipo effects the ones that are valid on call sites of the specific MI?

vtjnash · 2023-08-08T00:38:20Z

src/codegen.cpp

+        param.addDereferenceableAttr(props.union_bytes);
+        param.addAlignmentAttr(props.union_align);


Do we actually ensure this when making this sret alloca? Normally this is the upper bounds that are permitted to be assumed, not the actual values at runtime, but this might be a special case.

This what we do when we build the function in

julia/src/codegen.cpp

Line 7806 in 8b8da91

param.addDereferenceableAttr(returninfo.union_bytes);

Ah, fair. That is the only place it matters too. Elsewhere it just adds a little bit of extra overhead to the function declaration to track some extraneous attributes.

Doesn't it potenially give more information with how much memory an sret a function has ,i t's likely that the information is there on the pointer now but with opaque pointers it might not be?

vtjnash · 2023-08-08T00:41:33Z

src/codegen.cpp

+    if (is_decl && consistent == 0 && effect_free == 0 && nothrow == 1 && terminates == 1 && has_ptr_arg == false){
+        attr.addAttribute(Attribute::Speculatable); // This might not be legal because it assumes pointers can be null
+    }
+    attr.addAttribute(std::to_string(effects));


This seems like it should be ifndef NDEBUG?

Maybe we can add it as a comment to the module instead.

vtjnash · 2023-08-08T00:44:54Z

src/llvm-late-gc-lowering.cpp

-                                                } else if (SRet_gc == Alloca) {
-                                                    continue;
+                        //check if isSret is passed directly
+                        if (!(&F.arg_begin()[0] == CI->arg_begin()[0].get())) {


Even if the sret gets copied directly from somewhere else, it still needs to determine where the associated GC roots are, and make sure they are rooted.

So I added this as an attempt to figure an error out. What was happening here was that LLVM eliding everything about the sret and then complaining that it couldn't find it.

vchuravy · 2023-09-13T18:42:27Z

src/codegen.cpp

+    } else if (inaccessiblememonly == 2){
+        attr.addAttribute(Attribute::InaccessibleMemOrArgMemOnly);
+    }
+    if (notaskstate == 1) {


Is that read only task state or task state at all?

I.e. should we verify this?

So, from the docs

## `:notaskstate` The `:notaskstate` setting asserts that the method does not use or modify the local task state (task local storage, RNG state, etc.) and may thus be safely moved between tasks without observable results.

gbaraldi requested a review from vchuravy June 15, 2023 20:29

vchuravy requested review from wsmoses and pchintalapudi June 15, 2023 21:32

wsmoses reviewed Jun 16, 2023

View reviewed changes

gbaraldi marked this pull request as ready for review June 21, 2023 18:28

vtjnash reviewed Jul 6, 2023

View reviewed changes

src/codegen.cpp Outdated Show resolved Hide resolved

brenhinkeller added the compiler:llvm For issues that relate to LLVM label Aug 6, 2023

vtjnash reviewed Aug 8, 2023

View reviewed changes

vtjnash marked this pull request as draft August 8, 2023 00:45

Add julia level IPO effects to LLVM

1e8224e

gbaraldi force-pushed the llvm-effects branch from b8993d9 to 1e8224e Compare September 13, 2023 16:56

vchuravy reviewed Sep 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase of julia effects to LLVM #50188

Rebase of julia effects to LLVM #50188

gbaraldi commented Jun 15, 2023

gbaraldi commented Jun 16, 2023

pchintalapudi commented Jun 16, 2023

wsmoses Jun 16, 2023

gbaraldi Jun 16, 2023

wsmoses Jun 16, 2023

gbaraldi Jun 16, 2023

gbaraldi Jun 16, 2023

wsmoses Jun 16, 2023

gbaraldi commented Jun 16, 2023

pchintalapudi commented Jun 19, 2023

gbaraldi commented Jun 19, 2023

pchintalapudi commented Jun 19, 2023

gbaraldi commented Jun 19, 2023

gbaraldi commented Jun 26, 2023

nanosoldier commented Jun 26, 2023

gbaraldi commented Jun 27, 2023

gbaraldi commented Jun 27, 2023

gbaraldi commented Jul 10, 2023

gbaraldi commented Jul 10, 2023

nanosoldier commented Jul 11, 2023

nanosoldier commented Jul 11, 2023

vtjnash Aug 8, 2023

gbaraldi Aug 8, 2023

vtjnash Aug 8, 2023

gbaraldi Aug 8, 2023

vtjnash Aug 8, 2023

gbaraldi Aug 8, 2023

vtjnash Aug 8, 2023

gbaraldi Aug 8, 2023

vtjnash Aug 8, 2023

gbaraldi Aug 8, 2023

vchuravy Sep 13, 2023 •

edited

gbaraldi Sep 13, 2023

		param.addDereferenceableAttr(props.union_bytes);
		param.addAlignmentAttr(props.union_align);

Rebase of julia effects to LLVM #50188

Are you sure you want to change the base?

Rebase of julia effects to LLVM #50188

Conversation

gbaraldi commented Jun 15, 2023

gbaraldi commented Jun 16, 2023

pchintalapudi commented Jun 16, 2023

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

gbaraldi commented Jun 16, 2023

pchintalapudi commented Jun 19, 2023

gbaraldi commented Jun 19, 2023

pchintalapudi commented Jun 19, 2023

gbaraldi commented Jun 19, 2023

gbaraldi commented Jun 26, 2023

nanosoldier commented Jun 26, 2023

gbaraldi commented Jun 27, 2023

gbaraldi commented Jun 27, 2023

gbaraldi commented Jul 10, 2023

gbaraldi commented Jul 10, 2023

nanosoldier commented Jul 11, 2023

nanosoldier commented Jul 11, 2023

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

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

vchuravy Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy Sep 13, 2023 •

edited