correct codegen bugs introduced by allocation-hoisting-PR #45476

gbaraldi · 2022-05-27T02:15:09Z

Adresses some of the reviews for #45153

gbaraldi · 2022-05-31T21:05:09Z

@vtjnash How does this look to you? The mangling from emit_a_ccall gave errors so I got it from some passes downstream.

vtjnash · 2022-06-06T18:41:57Z

src/cgutils.cpp

@@ -901,7 +901,7 @@ static LoadInst *emit_nthptr_recast(jl_codectx_t &ctx, Value *v, ssize_t n, MDNo
 emit_bitcast(ctx, vptr, PointerType::get(type, 0)))));
 }

-static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &v);
+static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &v, bool is_promotable=false);


So promotion is now always forbidden implicitly?

Yes, the regression we were seeing was from something getting promoted when we called boxed on a separate part fo the of the compilation and then hoist something that shouldn't be hoisted.
In the case of the regression we were hosting a string allocation from inside an error to the main path of the function which then impeded an optimization.
From what I understand we should only hoist the allocation when we have nested structs and it's fields are promotable which we only check inside emit_new_struct

src/cgutils.cpp

Co-authored-by: Jameson Nash <[email protected]>

fredrikekre · 2022-06-14T11:48:04Z

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

nanosoldier · 2022-06-14T19:00:16Z

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(setenv(`git push`; dir="/nanosoldier/workdir/NanosoldierReports"), ProcessExited(1)) [1]

Unfortunately, the logs could not be uploaded.

vtjnash · 2022-06-14T21:25:39Z

https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/c299527_vs_6c97a48/report.md

gbaraldi · 2022-06-14T22:35:03Z

@vtjnash I'm not super accustomed to nanosoldier results, was this a particularly noisy benchmark or should I take a look at some more regressions.
The original regression found i.e more memory usage in skipmissing seems to have been fixed.

vtjnash · 2022-06-14T23:21:13Z

It looks like noise, though it wouldn't be too surprising if some of the differences were real. The memory changes are always real.

inkydragon · 2022-07-02T17:01:09Z

Maybe related to #45485.

julia> versioninfo()
Julia Version 1.9.0-DEV.883
Commit e4257cbc89 (2022-06-30 17:58 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 6 × Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.5 (ORCJIT, skylake)
  Threads: 1 on 6 virtual cores

julia> VERSION
v"1.9.0-DEV.883"

julia> struct MaybeTuple
           val::Union{Nothing, Tuple{Float32}}
       end

julia> MaybeTuple((0,))
Assertion failed: New->getType() == getType() && "replaceAllUses of value with new value of different type!", file /workspace/srcdir/llvm-project/llvm/lib/IR/Value.cpp, line 503

signal (22): SIGABRT
in expression starting at REPL[4]:1

gbaraldi · 2022-07-02T18:06:26Z

Yeah, this branch currently breaks unions.

edit: this commit should fix them

vchuravy · 2022-07-03T17:26:14Z

How close is this? Should we revert #45153 and then re-land it with these fixes?

gbaraldi · 2022-07-03T18:31:00Z

We might want to run pkgeval, but the issues related to the original pr are fixed by this

vchuravy · 2022-07-03T18:38:16Z

@nanosoldier runtests(ALL, vs = ":master", buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"], vs_buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"])

nanosoldier · 2022-07-04T06:52:02Z

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

gbaraldi · 2022-07-04T13:19:57Z

After a rebase I tested it locally and the failures seen weren't there

vtjnash · 2022-07-07T20:20:01Z

src/cgutils.cpp

@@ -3681,7 +3681,7 @@ static jl_cgval_t emit_new_struct(jl_codectx_t &ctx, jl_value_t *ty, size_t narg
 if (fval_info.typ == jl_bottom_type)
 return jl_cgval_t();
 // TODO: Use (post-)domination instead.
- bool field_promotable = !init_as_value && fval_info.promotion_ssa != -1 &&
+ bool field_promotable = !jl_is_uniontype(jtype) && !init_as_value && fval_info.promotion_ssa != -1 &&


It would be valid though if jl_field_isptr

The issue is that unions currently don't get their promotion point correctly, I'm not exactly what's needed to fix that, so for now, not allowing unions at all seems fine.

vtjnash · 2022-07-07T20:22:29Z

@nanosoldier runtests(["Ai4EComponentLib", "BioMASS", "BitInformation", "BloqadeODE", "Convex1d", "CryptoDashApp", "DifferentiableBackwardEuler", "EnsembleKalmanProcesses", "Evolutionary", "FiniteStateProjection", "Fronts", "GeoDatasets", "ImmersedLayers", "MRIgeneralizedBloch", "NumericalAlgorithms", "OpenStreetMapXPlot", "OptimKit", "PCRE2", "PlanningDomains", "PyRhodium", "Quiqbox", "RankRevealing", "RemoveLFS", "RipQP", "ShipMMG", "SimpleFWA", "TransferEntropy", "Transparency", "VoronoiGraph", "Zarr"], vs = ":master", buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"], vs_buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"])

vtjnash · 2022-07-07T20:22:42Z

@nanosoldier runbenchmarks(!"scalar", vs=":master")

nanosoldier · 2022-07-07T21:38:29Z

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

nanosoldier · 2022-07-08T02:17:23Z

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

vtjnash · 2022-07-08T04:29:12Z

@nanosoldier runbenchmarks(["array", "comprehension", ("collect", "Vector{Float64}")], vs=":master")

vtjnash · 2022-07-08T04:29:39Z

@nanosoldier runbenchmarks("array" && "comprehension" && ("collect", "Vector{Float64}"), vs=":master")

vtjnash · 2022-07-08T04:30:03Z

@nanosoldier runbenchmarks("array" && "comprehension", vs=":master")

nanosoldier · 2022-07-08T04:44:37Z

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

…45476) Co-authored-by: Jameson Nash <[email protected]>

gbaraldi added 5 commits May 26, 2022 23:13

Address reviews for older PR

4a99a31

Merge branch 'master' into fix-heaphoist

a3cb97b

Fix whitespace

508f292

Correct name mangling

ce8ff75

Implement different mangling approach

30f68bc

vtjnash reviewed Jun 6, 2022

View reviewed changes

src/cgutils.cpp Outdated Show resolved Hide resolved

gbaraldi added 2 commits June 6, 2022 16:06

Remove unused variable and small refactor

e366acb

Whitespace

76671d1

vtjnash reviewed Jun 9, 2022

View reviewed changes

src/cgutils.cpp Outdated Show resolved Hide resolved

change is_promotable to field_promotable

ac65255

Co-authored-by: Jameson Nash <[email protected]>

vtjnash changed the title ~~Address reviews for 45153~~ correct codegen bugs introduced by allocation-hoisting-PR Jun 10, 2022

vtjnash added the needs nanosoldier run This PR should have benchmarks run on it label Jun 10, 2022

gbaraldi added 4 commits June 22, 2022 11:50

Merge branch 'master' into fix-heaphoist

d645429

Add explicit check instead of assertion

4cac1d0

Fix the merge error ;(

7558018

Merge branch 'master' into fix-heaphoist

e4257cb

Explicetely not promote unions

2e8363b

vchuravy added this to the 1.9 milestone Jul 3, 2022

Merge branch 'master' into fix-heaphoist

9fcb436

gbaraldi added 2 commits July 6, 2022 19:29

Add a test for the issue found

088b36c

Fix whitespace

f8f7028

vtjnash reviewed Jul 7, 2022

View reviewed changes

vtjnash removed the needs nanosoldier run This PR should have benchmarks run on it label Jul 7, 2022

vtjnash approved these changes Jul 7, 2022

View reviewed changes

Merge branch 'master' into fix-heaphoist

d653492

maleadt merged commit d75843d into JuliaLang:master Jul 21, 2022

inkydragon mentioned this pull request Jul 22, 2022

Invalid conversion between floating-point tuples #45485

Closed

vtjnash referenced this pull request Jul 27, 2022

Throw descriptive BoundsError in copyto! (#46192)

b2bf56e

ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022

Correct codegen bugs introduced by allocation-hoisting-PR (JuliaLang#…

c5a8df8

…45476) Co-authored-by: Jameson Nash <[email protected]>

pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022

Correct codegen bugs introduced by allocation-hoisting-PR (JuliaLang#…

fbe9c5d

…45476) Co-authored-by: Jameson Nash <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

correct codegen bugs introduced by allocation-hoisting-PR #45476

correct codegen bugs introduced by allocation-hoisting-PR #45476

gbaraldi commented May 27, 2022

gbaraldi commented May 31, 2022

vtjnash Jun 6, 2022

gbaraldi Jun 6, 2022

fredrikekre commented Jun 14, 2022

nanosoldier commented Jun 14, 2022

vtjnash commented Jun 14, 2022

gbaraldi commented Jun 14, 2022

vtjnash commented Jun 14, 2022

inkydragon commented Jul 2, 2022

gbaraldi commented Jul 2, 2022 •

edited

Loading

vchuravy commented Jul 3, 2022

gbaraldi commented Jul 3, 2022

vchuravy commented Jul 3, 2022

nanosoldier commented Jul 4, 2022

gbaraldi commented Jul 4, 2022

vtjnash Jul 7, 2022

gbaraldi Jul 7, 2022

vtjnash commented Jul 7, 2022

vtjnash commented Jul 7, 2022

nanosoldier commented Jul 7, 2022

nanosoldier commented Jul 8, 2022

vtjnash commented Jul 8, 2022

vtjnash commented Jul 8, 2022

vtjnash commented Jul 8, 2022

nanosoldier commented Jul 8, 2022

correct codegen bugs introduced by allocation-hoisting-PR #45476

correct codegen bugs introduced by allocation-hoisting-PR #45476

Conversation

gbaraldi commented May 27, 2022

gbaraldi commented May 31, 2022

vtjnash Jun 6, 2022

Choose a reason for hiding this comment

gbaraldi Jun 6, 2022

Choose a reason for hiding this comment

fredrikekre commented Jun 14, 2022

nanosoldier commented Jun 14, 2022

vtjnash commented Jun 14, 2022

gbaraldi commented Jun 14, 2022

vtjnash commented Jun 14, 2022

inkydragon commented Jul 2, 2022

gbaraldi commented Jul 2, 2022 • edited Loading

vchuravy commented Jul 3, 2022

gbaraldi commented Jul 3, 2022

vchuravy commented Jul 3, 2022

nanosoldier commented Jul 4, 2022

gbaraldi commented Jul 4, 2022

vtjnash Jul 7, 2022

Choose a reason for hiding this comment

gbaraldi Jul 7, 2022

Choose a reason for hiding this comment

vtjnash commented Jul 7, 2022

vtjnash commented Jul 7, 2022

nanosoldier commented Jul 7, 2022

nanosoldier commented Jul 8, 2022

vtjnash commented Jul 8, 2022

vtjnash commented Jul 8, 2022

vtjnash commented Jul 8, 2022

nanosoldier commented Jul 8, 2022

gbaraldi commented Jul 2, 2022 •

edited

Loading