Allow constant-propagation to be disabled #42125

timholy · 2021-09-05T11:25:30Z

Our heuristics for constant propagation are imperfect (and probably
never will be perfect), and I've now seen many examples of
methods that no developer would ask to have const-propped get
that treatment. In some cases the cost for latency/precompilation
is very large. This renames @aggressive_constprop to @constprop
and allows two settings, :aggressive and :none.

Closes #38983 (presuming this will propagate to kw body methods, which were a huge deal in that DataFrames analysis)

Since we haven't released Julia 1.7 yet, I decided it was safe to do the rename. But if folks don't like that, we can have both @aggressive_constprop and @noconstprop. JuliaHub informs me that @aggressive_constprop is being used in half a dozen packages, but I can make PRs against them. And since it doesn't appear in our docs, it's not part of our stable interface anyway.

Our heuristics for constant propagation are imperfect (and probably never will be perfect), and I've now seen many examples of methods that no developer would ask to have const-propped get that treatment. In some cases the cost for latency/precompilation is very large. This renames `@aggressive_constprop` to `@constprop` and allows two settings, `:aggressive` and `:none`. Closes #38983 (presuming this will propagate to kw body methods)

src/ircode.c

aviatesk · 2021-09-06T10:13:57Z

presuming this will propagate to kw body methods, which were a huge deal in that DataFrames analysis

Actually we need to fix this as well ?

julia> Base.@aggressive_constprop foo(a; kws...) = a + length(kws)
foo (generic function with 1 method)

julia> (@which foo(10)).aggressive_constprop
true

julia> (@which foo(10; kw1 = nothing)).aggressive_constprop
false

The fix can be left for another PR though.

base/compiler/abstractinterpretation.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>

timholy · 2021-09-08T15:11:24Z

The kwfunc is missing, but the body function is covered:

julia> f = Base.bodyfunction(@which foo(10; kw1 = nothing))
#foo#11 (generic function with 1 method)

julia> m = only(methods(f))
var"#foo#11"(kws::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}}, ::typeof(foo), a) in Main at REPL[1]:1

julia> m.aggressive_constprop
true

One consolation is that the kwfunc should be pretty quick to infer for all methods, but there's no obvious limit on the body function inference time. Still, we definitely want to fix it.

timholy · 2021-09-09T07:15:24Z

One question: do I need to bump the serialization version number?

julia/stdlib/Serialization/src/Serialization.jl

Line 82 in 1326e4b

const ser_version = 15 # do not make changes without bumping the version #!

aggressive_constprop used to be serialized as a Bool, now constprop is serialized as a UInt8. But it's self-describing, so the version number issue would only affect different Julia versions (which presumably have a hard time communicating via the serializer anyway since internal structs, etc, change).

codecov · 2021-09-09T09:43:09Z

Codecov Report

Merging #42125 (1326e4b) into master (8812c5c) will decrease coverage by 0.14%.
The diff coverage is 85.85%.

❗ Current head 1326e4b differs from pull request most recent head 5f02e1d. Consider uploading reports for the commit 5f02e1d to get more accurate results

@@            Coverage Diff             @@
##           master   #42125      +/-   ##
==========================================
- Coverage   79.66%   79.51%   -0.15%     
==========================================
  Files         351      351              
  Lines       77567    77683     +116     
==========================================
- Hits        61790    61769      -21     
- Misses      15777    15914     +137

Impacted Files	Coverage Δ
base/binaryplatforms.jl	`81.90% <ø> (+50.60%)`	⬆️
base/compiler/types.jl	`64.28% <ø> (ø)`
base/shell.jl	`68.03% <ø> (ø)`
base/sysinfo.jl	`58.16% <0.00%> (ø)`
base/version.jl	`70.50% <0.00%> (+0.21%)`	⬆️
stdlib/REPL/src/REPL.jl	`34.29% <ø> (ø)`
base/compiler/abstractinterpretation.jl	`69.54% <50.00%> (-1.06%)`	⬇️
base/initdefs.jl	`42.65% <50.00%> (ø)`
base/floatfuncs.jl	`88.00% <80.00%> (-2.22%)`	⬇️
base/strings/util.jl	`86.03% <82.60%> (-0.96%)`	⬇️
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8812c5c...5f02e1d. Read the comment docs.

aviatesk · 2021-09-09T09:44:23Z

src/ircode.c

@@ -788,7 +788,7 @@ JL_DLLEXPORT jl_code_info_t *jl_uncompress_ir(jl_method_t *m, jl_code_instance_t

    jl_code_info_t *code = jl_new_code_info_uninit();
    uint8_t flags = read_uint8(s.s);
-    code->aggressive_constprop = !!(flags & (1 << 4));
+    code->constprop = (flags >> 4) & 3;


I may miss some basic understanding, but why do we need & 3 here (isn't flags >> 4 enough) ?

Another consideration here is, will we need to change this code when we add new flags (say when we add new boolean flag code->noinfer << 5 ? (that's why I suggested #42125 (comment))

This grabs both bits, without grabbing anything higher. So position 6 and higher are available and won't "contaminate" noinfer if they are nonzero. This is essentially the same as your suggestion but briefer.

One stylistic side note, instead of using the integer values directly, creating an enum and using named variables makes refactoring and understanding a tad easier.

Or maybe writing it 0b11 instead of 3, just so it's obviously "two bits"? If you created enums, I think you'd need them for both the shifts and the masks, and that would be almost a half-dozen enums for this one flag. Do you think that really would be worth it?

Ah, I get it. Thanks for your explanation.

I think I will like the idea of enum (we can do that in another PR).

Why not create a jl_code_info_flag_t or something with bit fields? We use it many other places like

julia/src/julia.h

Lines 168 to 175 in 2f00c5f

uint16_t how:2;

uint16_t ndims:9;

uint16_t pooled:1;

uint16_t ptrarray:1; // representation is pointer array

uint16_t hasptr:1; // representation has embedded pointers

uint16_t isshared:1; // data is shared by multiple Arrays

uint16_t isaligned:1; // data allocated with memalign

} jl_array_flags_t;

Do we need to keep the ABI compat for Serialization.jl?

I like that idea but this worries me:

C gives no guarantee of the ordering of fields within machine words, so if you do use them for the latter reason, you program will not only be non-portable, it will be compiler-dependent too.

yay, it looks much cleaner

Hmm... doesn't our GC already rely on the first field of the bitfield to be the lowerest bits?

julia/src/julia.h

Lines 97 to 109 in 50ab3a9

struct _jl_taggedvalue_bits {

uintptr_t gc:2;

};

JL_EXTENSION struct _jl_taggedvalue_t {

union {

uintptr_t header;

jl_taggedvalue_t *next;

jl_value_t *type; // 16-byte aligned

struct _jl_taggedvalue_bits bits;

};

// jl_value_t value;

};

i.e.., tag->bits.gc is used for extracting the lowest two bits of tag->type. (But yes, it'd be nice to be standard-compliant whenever possible.)

aviatesk · 2021-09-09T09:46:11Z

One question: do I need to bump the serialization version number?

Since v1.7 isn't released yet, I'd like to say we don't need to. Maybe @JeffBezanson has an idea on this ?

aviatesk · 2021-09-10T00:04:58Z

src/ircode.c

@@ -788,7 +788,7 @@ JL_DLLEXPORT jl_code_info_t *jl_uncompress_ir(jl_method_t *m, jl_code_instance_t

    jl_code_info_t *code = jl_new_code_info_uninit();
    uint8_t flags = read_uint8(s.s);
-    code->aggressive_constprop = !!(flags & (1 << 4));
+    code->constprop = (flags >> 4) & 3;


Ah, I get it. Thanks for your explanation.

I think I will like the idea of enum (we can do that in another PR).

timholy · 2021-09-10T09:01:06Z

I bumped the serialization number just as a precaution. If this passes I think we should merge.

timholy · 2021-09-10T12:02:22Z

xref JuliaLang/Compat.jl#752

adapt to JuliaLang/julia#42125

Our heuristics for constant propagation are imperfect (and probably never will be perfect), and I've now seen many examples of methods that no developer would ask to have const-propped get that treatment. In some cases the cost for latency/precompilation is very large. This renames `@aggressive_constprop` to `@constprop` and allows two settings, `:aggressive` and `:none`. Closes #38983 Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>

This also adds a depwarn for `@aggressive_constprop`, which seemed sensible to me, given that it will be removed in 1.8. replaces #42125

Our heuristics for constant propagation are imperfect (and probably never will be perfect), and I've now seen many examples of methods that no developer would ask to have const-propped get that treatment. In some cases the cost for latency/precompilation is very large. This renames `@aggressive_constprop` to `@constprop` and allows two settings, `:aggressive` and `:none`. Closes JuliaLang#38983 Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>

timholy added compiler:latency Compiler latency backport 1.7 labels Sep 5, 2021

aviatesk reviewed Sep 6, 2021

View reviewed changes

src/ircode.c Outdated Show resolved Hide resolved

aviatesk reviewed Sep 6, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

KristofferC mentioned this pull request Sep 8, 2021

release-1.7: Backports for 1.7-RC1 #41781

Merged

63 tasks

Apply suggestions from code review

e3f72cd

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>

aviatesk reviewed Sep 9, 2021

View reviewed changes

aviatesk approved these changes Sep 10, 2021

View reviewed changes

aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Sep 10, 2021

Define bitfield for code_info flags

5f02e1d

timholy merged commit 77572bc into master Sep 10, 2021

timholy deleted the teh/no_constprop branch September 10, 2021 11:00

timholy mentioned this pull request Sep 10, 2021

Support @constprop JuliaLang/Compat.jl#752

Merged

simeonschaub mentioned this pull request Sep 10, 2021

Add RuleConfig JuliaDiff/Diffractor.jl#45

Merged

simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021

adapt to JuliaLang/julia#42125

71d9202

simeonschaub added a commit to simeonschaub/Static.jl that referenced this pull request Sep 10, 2021

adapt to JuliaLang/julia#42125

e3e240c

simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021

adapt to JuliaLang/julia#42125

41e3b49

chriselrod added a commit to SciML/Static.jl that referenced this pull request Sep 10, 2021

Merge pull request #16 from simeonschaub/sds/fix_constprop

27ccdee

adapt to JuliaLang/julia#42125

simeonschaub added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021

adapt to JuliaLang/julia#42125

9709486

simeonschaub added a commit to simeonschaub/ArrayInterface.jl that referenced this pull request Sep 10, 2021

adapt to JuliaLang/julia#42125

59d564e

chriselrod added a commit to JuliaArrays/ArrayInterface.jl that referenced this pull request Sep 10, 2021

Merge pull request #204 from simeonschaub/sds/fix_consprop

dda3d00

adapt to JuliaLang/julia#42125

mcabbott added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Sep 10, 2021

Merge pull request #49 from JuliaDiff/sds/fix_constprop

0a98045

adapt to JuliaLang/julia#42125

DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 11, 2021

aviatesk mentioned this pull request Sep 11, 2021

release-1.7: set VERSION to 1.7.0-rc1 #42188

Merged

aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Sep 11, 2021

update to JuliaLang/julia#42125

7a676ea

aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Sep 11, 2021

update to JuliaLang/julia#42125

29981a8

This was referenced Sep 11, 2021

release-1.7: Backports for RC-1.7 #42217

Merged

release-1.7: Backports for 1.7-rc2 #42255

Merged

simeonschaub added a commit that referenced this pull request Sep 17, 2021

add @constprop for compatibility to 1.7

e4ae48f

This also adds a depwarn for `@aggressive_constprop`, which seemed sensible to me, given that it will be removed in 1.8. replaces #42125

simeonschaub mentioned this pull request Sep 17, 2021

add @constprop for compatibility to 1.7 #42298

Merged

KristofferC removed the backport 1.7 label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow constant-propagation to be disabled #42125

Allow constant-propagation to be disabled #42125

timholy commented Sep 5, 2021 •

edited

Loading

aviatesk commented Sep 6, 2021 •

edited

Loading

timholy commented Sep 8, 2021

timholy commented Sep 9, 2021 •

edited

Loading

codecov bot commented Sep 9, 2021 •

edited

Loading

aviatesk Sep 9, 2021

timholy Sep 9, 2021 •

edited

Loading

vchuravy Sep 9, 2021

timholy Sep 9, 2021

aviatesk Sep 10, 2021

tkf Sep 10, 2021

timholy Sep 10, 2021 •

edited

Loading

aviatesk Sep 10, 2021

tkf Sep 10, 2021 •

edited

Loading

aviatesk commented Sep 9, 2021

aviatesk Sep 10, 2021

timholy commented Sep 10, 2021

timholy commented Sep 10, 2021

	uint16_t how:2;
	uint16_t ndims:9;
	uint16_t pooled:1;
	uint16_t ptrarray:1; // representation is pointer array
	uint16_t hasptr:1; // representation has embedded pointers
	uint16_t isshared:1; // data is shared by multiple Arrays
	uint16_t isaligned:1; // data allocated with memalign
	} jl_array_flags_t;

	struct _jl_taggedvalue_bits {
	uintptr_t gc:2;
	};

	JL_EXTENSION struct _jl_taggedvalue_t {
	union {
	uintptr_t header;
	jl_taggedvalue_t *next;
	jl_value_t *type; // 16-byte aligned
	struct _jl_taggedvalue_bits bits;
	};
	// jl_value_t value;
	};

Allow constant-propagation to be disabled #42125

Allow constant-propagation to be disabled #42125

Conversation

timholy commented Sep 5, 2021 • edited Loading

aviatesk commented Sep 6, 2021 • edited Loading

timholy commented Sep 8, 2021

timholy commented Sep 9, 2021 • edited Loading

codecov bot commented Sep 9, 2021 • edited Loading

Codecov Report

aviatesk Sep 9, 2021

Choose a reason for hiding this comment

timholy Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

vchuravy Sep 9, 2021

Choose a reason for hiding this comment

timholy Sep 9, 2021

Choose a reason for hiding this comment

aviatesk Sep 10, 2021

Choose a reason for hiding this comment

tkf Sep 10, 2021

Choose a reason for hiding this comment

timholy Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

aviatesk Sep 10, 2021

Choose a reason for hiding this comment

tkf Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

aviatesk commented Sep 9, 2021

aviatesk Sep 10, 2021

Choose a reason for hiding this comment

timholy commented Sep 10, 2021

timholy commented Sep 10, 2021

timholy commented Sep 5, 2021 •

edited

Loading

aviatesk commented Sep 6, 2021 •

edited

Loading

timholy commented Sep 9, 2021 •

edited

Loading

codecov bot commented Sep 9, 2021 •

edited

Loading

timholy Sep 9, 2021 •

edited

Loading

timholy Sep 10, 2021 •

edited

Loading

tkf Sep 10, 2021 •

edited

Loading