Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codegen: ensure i1 bool is widened to i8 before storing #52189

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 16, 2023

Teach value_to_pointer to convert primitive types to their stored representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to anywhere inside any struct for changing any bitwidth to a multiple of 8 bytes. This would change a vector like <2 x i4> from occupying i8 to i16 (c.f. LLVM's LangRef), if such an operation were expressible in Julia today. And take this opportunity to do a bit of code cleanup, now that codegen is better and using helpers from LLVM.

Fixes #52127

For backporting this fix to older versions, the minimal diff to use is:

diff --git a/src/codegen.cpp b/src/codegen.cpp
index 5ad4eb4e6e..b89a9f5ca6 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -1935,6 +1935,8 @@ static bool valid_as_globalinit(const Value *v) {
 static inline jl_cgval_t value_to_pointer(jl_codectx_t &ctx, Value *v, jl_value_t *typ, Value *tindex)
 {
     Value *loc;
+    if (v->getType()->isIntegerTy(1))
+        v = ctx.builder.CreateZExt(v, IntegerType::get(v->getContext(), 8));
     if (valid_as_globalinit(v)) { // llvm can't handle all the things that could be inside a ConstantExpr
         assert(jl_is_concrete_type(typ)); // not legal to have an unboxed abstract type
         loc = get_pointer_to_constant(ctx.emission_context, cast<Constant>(v), Align(julia_alignment(typ)), "_j_const", *jl_Module);

Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127
@vtjnash vtjnash added the backport 1.10 Change should be backported to the 1.10 release label Nov 16, 2023
@giordano giordano added the compiler:codegen Generation of LLVM IR and native code label Nov 16, 2023
@oscardssmith oscardssmith added the bugfix This change fixes an existing bug label Nov 16, 2023
@vtjnash
Copy link
Member Author

vtjnash commented Nov 16, 2023

CI is good. @JeffBezanson is this good to merge?

@wsmoses
Copy link
Contributor

wsmoses commented Nov 17, 2023

@vtjnash could this code be exported rather than static. We have this implemented in Enzyme.jl already and it would be nice to not need to duplicate it: https://github.com/EnzymeAD/Enzyme.jl/blob/4329953f6f707982f841c6ed6bc164dd5ae65094/src/compiler.jl#L2095

@JeffBezanson JeffBezanson merged commit 9aa7980 into master Nov 17, 2023
7 of 10 checks passed
@JeffBezanson JeffBezanson deleted the jn/52127 branch November 17, 2023 18:58
@vtjnash
Copy link
Member Author

vtjnash commented Nov 17, 2023

@wsmoses It looks like you already have a version of it. It shouldn't be that necessary to use, as we use it just before we fully initialize the struct with actual content. I just prefer avoiding letting LLVM see any non-integral UndefValue and taking too many licenses with that (since we could attempt to observe them later on in gc-lowering), even though that is unlikely (and I think we would ignore / correct that later anyways)

@KristofferC KristofferC mentioned this pull request Nov 27, 2023
39 tasks
KristofferC pushed a commit that referenced this pull request Nov 27, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127

(cherry picked from commit 9aa7980)
KristofferC pushed a commit that referenced this pull request Nov 27, 2023
Teach value_to_pointer to convert primitive types to their stored
representation first, to avoid exposing undef bits later (via memcpy).

Take this opportunity to also generalizes the support for zext Bool to
anywhere inside any struct for changing any bitwidth to a multiple of 8
bytes. This would change a vector like <2 x i4> from occupying i8 to i16
(c.f. LLVM's LangRef), if such an operation were expressible in Julia
today. And take this opportunity to do a bit of code cleanup, now that
codegen is better and using helpers from LLVM.

Fixes #52127

(cherry picked from commit 9aa7980)
KristofferC added a commit that referenced this pull request Dec 2, 2023
Backported PRs:
- [x] #51213 <!-- Wait for other threads to finish compiling before
exiting -->
- [x] #51520 <!-- Make allocopt respect the GC verifier rules with non
usual address spaces -->
- [x] #51598 <!-- Use a simple error when reporting sysimg load
failures. -->
- [x] #51757 <!-- fix parallel peakflop usage -->
- [x] #51781 <!-- Don't make pkgimages global editable -->
- [x] #51848 <!-- allow finalizers to take any locks and yield during
exit -->
- [x] #51847 <!-- add missing wait during Timer and AsyncCondition close
-->
- [x] #50824 <!-- Add some aliasing warnings to docstrings for mutating
functions in Base -->
- [x] #51885 <!-- remove chmodding the pkgimages -->
- [x] #50207 <!-- [devdocs] Improve documentation about building
external forks of LLVM -->
- [x] #51967 <!-- further fix to the new promoting method for
AbstractDateTime subtraction -->
- [x] #51980 <!-- macroexpand: handle const/atomic struct fields
correctly -->
- [x] #51995 <!-- [Artifacts] Pass artifacts dictionary to
`ensure_artifact_installed` dispatch -->
- [x] #52098 <!-- Fix errors in `sort` docstring -->
- [x] #52136 <!-- Bump JuliaSyntax to 0.4.7 -->
- [x] #52140 <!-- Make c func `abspath` consistent on Windows. Fix
tracking path conversion. -->
- [x] #52009 <!-- fix completion that resulted in startpos of 0 for `\\
-->
- [x] #52192 <!-- cap the number of GC threads to number of cpu cores
-->
- [x] #52206 <!-- Make have_fma consistent between interpreter and
compiled -->
- [x] #52027 <!-- fix Unicode.julia_chartransform for Julia 1.10 -->
- [x] #52217 <!-- More helpful error message for empty `cpu_target` in
`Base.julia_cmd` -->
- [x] #51371 <!-- Memoize `cwstring` when used for env lookup /
modification on Windows -->
- [x] #52214 <!-- Turn Method Overwritten Error into a PrecompileError
-- turning off caching -->
- [x] #51895 <!-- Devdocs on fixing precompile hangs, take 2 -->
- [x] #51596 <!-- Reland "Don't mark nonlocal symbols as hidden"" -->
- [x] #51834 <!-- [REPLCompletions] allow symbol completions within
incomplete macrocall expression -->
- [x] #52010 <!-- Revert "Support sorting iterators (#46104)" -->
- [x] #51430 <!-- add support for async backtraces of Tasks on any
thread -->
- [x] #51471 <!-- Fix segfault if root task is NULL -->
- [x] #52194 <!-- Fix multiversioning issues caused by the parallel llvm
work -->
- [x] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are
now first class -->
- [x] #52030 <!-- Bump Statistics -->
- [x] #52189 <!-- codegen: ensure i1 bool is widened to i8 before
storing -->
- [x] #52228 <!-- Widen diagonal var during `Type` unwrapping in
`instanceof_tfunc` -->
- [x] #52182 <!-- jitlayers: replace sharedbytes intern pool with one
that respects alignment -->

Contains multiple commits, manual intervention needed:
- [ ] #51092 <!-- inference: fix bad effects for recursion -->

Non-merged PRs with backport label:
- [ ] #52196 <!-- Fix creating custom log level macros -->
- [ ] #52170 <!-- fix invalidations related to `ismutable` -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Int(true) == 255 with broadcasting?
7 participants