Fix integer and complex arguments to unary C math functions - #1051
Open
MaxFreedomPollard wants to merge 2 commits into
Open
MaxFreedomPollard wants to merge 2 commits into
MaxFreedomPollard wants to merge 2 commits into
Conversation
CMathCallable.with_types picked the precision suffix from the type of the argument before integers were promoted to float32, so an integer argument never matched any of the float branches and always reached the type-error fallback at loopy/target/c/__init__.py:647, which then printed the promoted type: "sqrt does not support type <class 'numpy.float32'>". The comparison against the real part of the type dates to 0f15b00, where the unary branch still only accepted arguments of kind "f" or "c", so an integer could not reach it; the guard did not come along when the code moved into the callable. out[i] = sqrt(i) has therefore worked on OpenCLTarget and failed on CTarget, CudaTarget and ISPCTarget. The result type of abs, real and imag was determined after the name had already picked up its "f" suffix (line 649 vs the suffix block above it), so "absf" was no longer in the ["abs", "real", "imag"] list and complex64 was typed as returning complex64. complex128 gets no suffix and was typed correctly, which is why fmax(abs(a[i]), 1.0) compiled in double precision and raised "fmax does not support complex numbers" in single precision. Moving the integer promotion above the computation of the real type and the result type above the suffix block repairs both. Two further points: - Integer abs now stays integral, as OpenCLCallable.with_types already keeps it. Sending it through float32 would drop bits from an int64 and would make abs unusable in an array index. C has abs/labs/llabs but no unsigned form, so this emits an lpy_abs_<type> preamble in the manner of the existing lpy_min/lpy_max helpers, with the type name in the preamble tag because process_preambles deduplicates on the tag and one kernel can need two widths. - The promotion assigned the class np.float32 rather than an np.dtype, so the dtype.kind access further down would have raised AttributeError had the branch ever been reachable. It now assigns np.dtype(np.float32). real, imag and conj of an integer are deliberately left out of the promotion and still raise, now naming the argument type rather than float32.
CFamilyTarget.dtype_to_typename carries no annotations, so basedpyright reports every call to it as "Type of "dtype_to_typename" is partially unknown" (reportUnknownMemberType). The three such calls that CMathCallable.generate_preambles already made are recorded in .basedpyright/baseline.json, so the call added for the lpy_abs_<type> preamble is a fourth occurrence, which the checker counts as new and which fails the basedpyright job. dtype_to_typename is a one-line forward to get_dtype_registry().dtype_to_ctype, marked "These kind of shouldn't be here." where it is defined, while CFamilyTarget.get_dtype_registry and DTypeRegistry.dtype_to_ctype are both annotated. Going through the registry, as loopy.target.c.codegen.expression and loopy.target.ispc already do, is the same call at run time and is fully typed, so the new code needs no baseline entry. The generated C is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CMathCallable.with_typescomputesreal_dtypefrom the argument type at loopy/target/c/init.py:632 and promotes integers to float32 only afterwards, at :634. For an integer argumentreal_dtypeis still the integer type, so none of the float64 / float32 / float128 branches at :639 to :645 match, and control always reaches theraise LoopyTypeErrorat :647. That fallback prints the already promoted type, so the message issqrt does not support type <class 'numpy.float32'>, naming a type that is in fact supported. The comparison against the real part of the argument type came in with 0f15b00, "separate pyopencl complex codegen from C99's complex codegen", where the unary branch was still gated onarg_dtypes[0].numpy_dtype.kind in "fc"so an integer could not reach it. The gate did not come along when that code moved intoCMathCallable: fd179f4, "Fix bad merge in cmathcallable to fix complex support", already has the ordering this branch repairs, withreal_dtypecomputed above an integer promotion that nothing can reach.Every unary C math function is affected on
CTarget,CudaTargetandISPCTarget: sqrt, sin, cos, exp, log, abs and the rest.out[i] = sqrt(i)generates code onOpenCLTargetand raises onCTarget, so this is a regression against a sibling target.The same branch has a second ordering defect.
result_dtypeis chosen at :649, afternamehas picked up itsfsuffix at :642. For a complex64 argument the name is alreadyabsf,realforimagfby then, thename in ["abs", "real", "imag"]test fails, andresult_dtypecomes out complex64. complex128 takes no suffix and is typed correctly, which is the asymmetry:fmax(abs(a[i]), 1.0)compiles for complex128 and raisesfmax does not support complex numbersfor complex64, and an auto typed temporary holdingabs(a[i])is declaredfloat complexin the generated C even thoughcabsfreturns a real float.The change moves the integer promotion above the
real_dtypecomputation and theresult_dtypechoice above the suffix block. Three smaller repairs follow. The promotion now assignsnp.dtype(np.float32)rather than the classnp.float32, so thedtype.kindaccess at :651 is valid on that path; with the class it would have raisedAttributeErrorhad the branch ever been reachable. Integerabsis kept integral instead of being promoted, the wayOpenCLCallable.with_typesalready keeps it at loopy/target/opencl.py:253; sending it through float32 would drop bits from an int64 and would makea[abs(5-i)]fail withNon-integral array indices. The two targets still differ in signedness, because OpenCL returnsto_unsigned_dtype(dtype)to match OpenCL C 2.2 section 6.13.3 while C's abs, labs and llabs return a signed type, soabs(a[i])on an int32 yields int32 here and uint32 onOpenCLTarget. C has no unsigned form of abs, so this emits anlpy_abs_<type>preamble in the manner of thelpy_minandlpy_maxhelpers a few lines above it, with the type name in the tag rather than in the body alone becauseprocess_preamblesat loopy/codegen/result.py:46 deduplicates on the tag and one kernel can need two widths at once.Left alone:
real,imagandconjof an integer stay out of the promotion and still raise, now naming the argument type instead of float32. Integerminandmaxare untouched, they are handled by the separate branch at :702.OpenCLTargetandPyOpenCLTargetare untouched.Four tests in test/test_target.py, 24 cases in total:
test_c_math_integer_argumentover sqrt, sin, cos, exp and log onCTargetandCudaTarget;test_c_math_integer_absover int32, int64 and uint32 on both targets;test_c_math_integer_abs_helperon both targets, covering the array index use and a kernel that needs the int32 and int64 helpers at once; andtest_c_math_complex_valued_argumentover abs, real and imag for complex64 and complex128, where the complex128 cases are the controls that already passed. They only generate code and infer types, so they need no OpenCL device, the same justification as the test in #1048.The pre-existing failures are identical with and without the change, down to the test ids. They are
pyopencl._cl.LogicError: clCreateKernel failed: INVALID_KERNELfrom Apple's OpenCL on an M3 and are unrelated. The basedpyright error lists match line for line, and the single added warning isType of "dtype_to_typename" is partially unknownon thetarget.dtype_to_typename(dtype)call that builds the new preamble. An end to end numeric check throughExecutableCTargetwith a real compiler givesabs(int64) -> [0 1 2 3] int64,sqrt(int32) -> [0. 1. 1.4142135 1.7320508] float32andabs(complex64) -> [0. 1.4142135 2.828427 4.2426405] float32, all matching numpy; the same script on 8807bdd dies at loopy/target/c/init.py:647. Platform: macOS 26.2 (Darwin 25.2.0), Python 3.12.13, numpy 2.5.3, islpy 2026.2.2, pyopencl 2026.1.4, ruff 0.16.7, basedpyright 1.40.1.