Fix incorrect `byval` and other attributes on LLVM 14 #950

apmasell · 2023-05-17T20:48:22Z

Some attributes now need type information. This preserves it.

Closes #946

Some attributes now need type information. This preserves it.

esc · 2023-05-18T08:50:58Z

Two high-level questions from my initial look:

Can the tests be made to invoke LLVM and check against the produced output, in addition to checking for the "canned" result we have now?
What happens to the additional parameter attributes that were added in LLVM 14 -- like byref? Will you add support for these too?

apmasell · 2023-05-18T13:33:00Z

For the first, I don't know how to do that given the way I can get the plumbing to work with pickle. If you have a design, I'm happy to hear it.

For the second, until we have a true way to control LLVM backend version, we should only do that if we raise the minimum LLVM version. Are we prepared to do that in a patch release?

thomasfire · 2023-05-19T03:42:54Z

For the first, I don't know how to do that given the way I can get the plumbing to work with pickle. If you have a design, I'm happy to hear it.

Is it possible to integrate these pieces of IR from tests to some function and see if LLVM compiles it?

apmasell · 2023-05-19T12:44:21Z

That's what the current test does.

esc · 2023-05-25T10:35:33Z

OK, so just to double check, the way this works now:

a) We have code to add the types to the Parameter Attributes
b) The "canned" tests have been updated, but they only test for the sret parameter attribute
c) We have a generated test, where we test byval -- such that we generate some IR and then ensure that the given version of LLVM can indeed parse this IR

The question I have, and I have no idea if this even needs to be asked: so it seems that we only check for sret and byval, but these are not the only parameter attributes? Do we need to -- or should we -- test the other attributes too?

The second question that I have is if we can ask anyone from the artiq project to check this PR with their code? This is where these issues were originally detected, and perhaps this is a good test case for this PR: m-labs/artiq#2050 @thomasfire would you be able to facilitate this?

sklam · 2023-05-26T16:22:52Z

The question I have, and I have no idea if this even needs to be asked: so it seems that we only check for sret and byval, but these are not the only parameter attributes? Do we need to -- or should we -- test the other attributes too?

I think we should proactively test for all the parameter attributes. The test should be easy to write and we just need to send it through parse_assembly().

As titled.

apmasell · 2023-05-26T21:41:06Z

Added.

thomasfire · 2023-05-29T03:53:55Z

I've tested our code against this branch, and would confirm that this fixes the issue

esc · 2023-05-29T10:40:36Z

I've tested our code against this branch, and would confirm that this fixes the issue

Awesome, thank you for your feedback!

@thomasfire would you like to be added to our RC-Testers group on our discourse:

https://numba.discourse.group/

it's a group of folks who are interested in new Numba/llvmlite release candidates, perhaps this could be of interest to you? It would perhaps help with future llvmlite testing?

thomasfire · 2023-05-30T02:08:46Z

would you like to be added to our RC-Testers group on our discourse:

Yes, you can add me, username is same as github's

esc

I have a suggestion to make about using slightly more Pythonic syntax.

llvmlite/ir/values.py

Co-authored-by: esc <esc@users.noreply.github.com>

esc · 2023-06-01T11:16:25Z

would you like to be added to our RC-Testers group on our discourse:

Yes, you can add me, username is same as github's

You should have received an invite now.

esc · 2023-06-01T12:15:57Z

Since this also supports LLVM 11 -- I was wanting to build this PR and test against that. However, I am having trouble getting a development environment setup. Mostly I am troubled with conda environment conflicts.

esc · 2023-06-01T12:26:06Z

So I tried this locally, by applying the patches from this PR to the https://github.com/numba/llvmlite/tree/release0.40 branch. I then compiled this against an llvmdev=11 from the numba channel on anaconda.org.

When running the tests, I receive:

======================================================================
ERROR: test_arg_attributes (llvmlite.tests.test_ir.TestBuildInstructions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/vhaenel/git/llvmlite/llvmlite/tests/test_ir.py", line 1852, in test_arg_attributes
    llvm.parse_assembly(gen_code(attr_name))
  File "/Users/vhaenel/git/llvmlite/llvmlite/binding/module.py", line 25, in parse_assembly
    raise RuntimeError("LLVM IR parsing error\n{0}".format(errmsg))
RuntimeError: LLVM IR parsing error
<string>:5:24: error: expected ')' at end of argument list
define i32 @"sum"(i32* byref %".1", i32 %".2")

As titled.

apmasell · 2023-06-02T01:21:34Z

I've added a commit that should make it work properly on LLVM 11. Some of the attributes being tested weren't supported on LLVM 11.

esc · 2023-06-02T11:28:06Z

I've added a commit that should make it work properly on LLVM 11. Some of the attributes being tested weren't supported on LLVM 11.

Thank you, I can confirm that with the commit efb856a the llvmlite test suite passes with LLVM 11.

esc · 2023-06-02T12:15:32Z

So I tried running the Numba main test-suite against an llvmlite from this PR built against LLVM 11 -- I received one error, but I am not sure this is related to my work:

======================================================================
FAIL: test_parfor_race_1 (numba.tests.test_parfors.TestPrangeSpecific)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/esc/git/numba/numba/tests/test_parfors.py", line 4091, in test_parfor_race_1
    self.assertIn(expected_msg, str(warning_obj.message))
AssertionError: 'Variable k used in parallel loop may be written to simultaneously by multiple workers and may result in non-deterministic or unintended results.' not found in 'A Numba config file is found but YAML parsing capabilities appear to be missing. To use this feature please install `pyyaml`. e.g. `conda install pyyaml`.'

apmasell · 2023-06-02T13:09:48Z

That's exciting and I desperately hope not related to these changes.

esc · 2023-06-02T13:20:22Z

That's exciting and I desperately hope not related to these changes.

No, it wasn't, it was a side effect of not having pyyaml installed.

esc · 2023-06-02T14:36:33Z

I am now hitting:

.Assertion failed: (isInt<33>(Addend) && "Invalid page reloc value."), function encodeAddend, file /Users/ci/miniconda3-arm64/conda-bld/llvmdev_1643905487494/work/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldMachOAArch64.h, line 210.
Fatal Python error: Aborted

Current thread 0x0000000104d34580 (most recent call first):
  File "/Users/vhaenel/git/llvmlite/llvmlite/binding/ffi.py", line 152 in __call__
  File "/Users/vhaenel/git/llvmlite/llvmlite/binding/executionengine.py", line 92 in finalize_object
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 1060 in wrapper
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 999 in _finalize_specific
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 797 in _finalize_final_module

Which I believe is known issue on Mac ARM64?

esc · 2023-06-02T15:26:55Z

I am now hitting:

.Assertion failed: (isInt<33>(Addend) && "Invalid page reloc value."), function encodeAddend, file /Users/ci/miniconda3-arm64/conda-bld/llvmdev_1643905487494/work/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldMachOAArch64.h, line 210.
Fatal Python error: Aborted

Current thread 0x0000000104d34580 (most recent call first):
  File "/Users/vhaenel/git/llvmlite/llvmlite/binding/ffi.py", line 152 in __call__
  File "/Users/vhaenel/git/llvmlite/llvmlite/binding/executionengine.py", line 92 in finalize_object
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 1060 in wrapper
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 999 in _finalize_specific
  File "/Users/vhaenel/git/numba/numba/core/codegen.py", line 797 in _finalize_final_module

Which I believe is known issue on Mac ARM64?

It is this one here: numba/numba#8567

OK, I ran it again and it seems like the issue is transient, so I would say this did indeed pass a spot check, with LLVM 11.

esc

Question: can we compile?

esc · 2023-06-05T08:45:26Z

llvmlite/tests/test_ir.py

+            'zeroext',
+        ) + supplemental:
+            # If this parses, we emitted the right byval attribute format
+            llvm.parse_assembly(gen_code(attr_name))


Would it be possible to try to compile this too? Or is the test-code not self-sufficient?

We're applying attributes in ways that don't really make sense, so I'm guessing most of these would be ignored at best or cause compilation problems. We'd need to design a test case for each attribute.

ok, that makes sense, thank you.

esc · 2023-06-05T16:42:18Z

This is now going in, thank you for the patch!

Fix incorrect `byval` and other attributes on LLVM 14

apmasell added the 2 - In Progress label May 17, 2023

Fix incorrect byval and other attributes on LLVM 14

bac9538

Some attributes now need type information. This preserves it.

apmasell added 3 - Ready for Review and removed 2 - In Progress labels May 17, 2023

esc added 4 - Waiting on author and removed 3 - Ready for Review labels May 18, 2023

esc added this to the v0.40.1-rc1 milestone May 25, 2023

apmasell added 4 - Waiting on reviewer and removed 4 - Waiting on author labels May 26, 2023

esc added 4 - Waiting on author 4 - Waiting on reviewer and removed 4 - Waiting on reviewer 4 - Waiting on author labels May 26, 2023

Include and test all LLVM14 argument attributes

9814f11

As titled.

This was referenced May 30, 2023

Update to LLVM 14 and llvmlite 40 m-labs/artiq#2097

Merged

python-llvmlite: Fix attributes syntax mismatch on LLVM 14 msys2/MINGW-packages#17370

Closed

esc requested changes May 31, 2023

View reviewed changes

llvmlite/ir/values.py Outdated Show resolved Hide resolved

Make syntax more Pythonic

4137b6b

Co-authored-by: esc <esc@users.noreply.github.com>

Handle argument attributes introduced in LLVM 14

efb856a

As titled.

esc requested changes Jun 5, 2023

View reviewed changes

esc merged commit 70f057b into numba:main Jun 5, 2023
18 checks passed

esc added 5 - Ready to merge and removed 4 - Waiting on reviewer labels Jun 5, 2023

apmasell deleted the llvm14_attributes branch June 6, 2023 00:59

esc added a commit to esc/llvmlite that referenced this pull request Jun 6, 2023

Merge pull request numba#950 from apmasell/llvm14_attributes

28fd236

Fix incorrect `byval` and other attributes on LLVM 14

esc mentioned this pull request Jun 6, 2023

llvmlite v0.40.1-rc1 Checklist #955

Closed

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect `byval` and other attributes on LLVM 14 #950

Fix incorrect `byval` and other attributes on LLVM 14 #950

apmasell commented May 17, 2023 •

edited

esc commented May 18, 2023

apmasell commented May 18, 2023

thomasfire commented May 19, 2023

apmasell commented May 19, 2023

esc commented May 25, 2023

sklam commented May 26, 2023

apmasell commented May 26, 2023

thomasfire commented May 29, 2023

esc commented May 29, 2023

thomasfire commented May 30, 2023

esc left a comment

esc commented Jun 1, 2023

esc commented Jun 1, 2023

esc commented Jun 1, 2023

apmasell commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

apmasell commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

esc left a comment

esc Jun 5, 2023

apmasell Jun 5, 2023

esc Jun 5, 2023

esc commented Jun 5, 2023

Fix incorrect byval and other attributes on LLVM 14 #950

Fix incorrect byval and other attributes on LLVM 14 #950

Conversation

apmasell commented May 17, 2023 • edited

esc commented May 18, 2023

apmasell commented May 18, 2023

thomasfire commented May 19, 2023

apmasell commented May 19, 2023

esc commented May 25, 2023

sklam commented May 26, 2023

apmasell commented May 26, 2023

thomasfire commented May 29, 2023

esc commented May 29, 2023

thomasfire commented May 30, 2023

esc left a comment

Choose a reason for hiding this comment

esc commented Jun 1, 2023

esc commented Jun 1, 2023

esc commented Jun 1, 2023

apmasell commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

apmasell commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

esc commented Jun 2, 2023

esc left a comment

Choose a reason for hiding this comment

esc Jun 5, 2023

Choose a reason for hiding this comment

apmasell Jun 5, 2023

Choose a reason for hiding this comment

esc Jun 5, 2023

Choose a reason for hiding this comment

esc commented Jun 5, 2023

Fix incorrect `byval` and other attributes on LLVM 14 #950

Fix incorrect `byval` and other attributes on LLVM 14 #950

apmasell commented May 17, 2023 •

edited