LLVM and SPIRV-LLVM-Translator pulldown (WW29) #10427
Conversation
When I landed the EmulateUnsupportedFloats, I'd negligently included an assert that needed to run for the pass to be correct. Previous emergency fix commits removed the assert. This commit re-adds the "can't happen" testing as an emitOpError() and aborting the rewrite, thus allowing it to function in no-assertions builds. Reviewed By: kuhar Differential Revision: https://reviews.llvm.org/D155088
Combine mul (f32) + fptrunc (f32->f16) to "v_fma_mixlo_f16 mulSrc1, mulSrc2, 0". Differential Revision: https://reviews.llvm.org/D153544 Reviewers: arsenm, foad
SubOpAliases maps a sub-operand name to the respective operand's index
and the sub-operand number within this operand. The operand index is
used for the Operands array.
Currently MIOperandNo is used as the operand index, which is not
correct. For example, if there are 2 operands with 3 sub-operands each:
(ins (bdladdr12onlylen4 $B1, $D1, $L1):$BDL1,
(bdladdr12onlylen4 $B2, $D2, $L2):$BDL2)
then B2's operand index will be 3, but the correct value is 1.
Reviewed By: jyknight
Differential Revision: https://reviews.llvm.org/D155158
There was a bug with the -funderscoring / -fno-underscoring options from (https://reviews.llvm.org/D140795) that prevented the driver option from controlling the underscoring behaviour and instead the behaviour could only be controlled by the pass option instead of the driver option. The driver test case did not catch the bug and also needed to be updated. Reviewed By: awarzynski Differential Revision: https://reviews.llvm.org/D155042
…nfc] This is a subset of Luke's D155063. I'm splitting pieces and landing them in the process of convincing myself all the individual transforms are in fact correct. In this case, we're simplifying based on the assumption that all of our vmerge operands have mask operands. This is a fundemental property of a vmerge.
Remove duplicate of the getDesignatorNameIfDataRef() function. Reviewed By: kiranchandramohan Differential Revision: https://reviews.llvm.org/D155105
Check the combination of reduction operator and types. This is currently not checking common block and composite types. Depends on D155105 Reviewed By: razvanlupusoru Differential Revision: https://reviews.llvm.org/D155106
Removing the x86-specific node helps further folding and improves commutativity
This seems to match https://gcc.gnu.org/install/specific.html#powerpc-x-eabi It seems that anything with OS `none` (although that doesn’t seem to be distinguished from `unknown`) or with environment `eabi` should be treated as bare-metal. Since this seems to have been handled on a case-by-case basis in the past ([arm](https://reviews.llvm.org/D33259), [riscv](https://reviews.llvm.org/D91442), [aarch64](https://reviews.llvm.org/D111134)), what I am proposing here is to add another case to the list to also handle `powerpc[64][le]-unknown-unknown-eabi` using the `BareMetal` toolchain, following the example of the existing cases. (We don’t care about powerpc64 and powerpc[64]le, but it seemed appropriate to lump them in.) At Indel, we have been building bare-metal embedded applications that run on custom PowerPC and ARM systems with Clang and LLD for a couple of years now, using target triples `powerpc-indel-eabi`, `powerpc-indel-eabi750`, `arm-indel-eabi`, `aarch64-indel-eabi` (which I just learned from D153430 is wrong and should be `aarch64-indel-elf` instead, but that’s a different matter). This has worked fine for ARM, but for PowerPC we have been unable to call the linker (LLD) through the Clang driver, because it would insist on calling GCC as the linker, even when told `-fuse-ld=lld`. That does not work for us, there is no GCC around. Instead we had to call `ld.lld` directly, introducing some special cases in our build system to translate between linker-via-driver and linker-called-directly command line arguments. I have now dug into why that is, and found that the difference between ARM and PowerPC is that `arm-indel-eabi` hits a special case that causes the Clang driver to instantiate a `BareMetal` toolchain that is able to call LLD and works the way we need, whereas `powerpc-indel-eabi` lands in the default case of a `Generic_ELF` (subclass of `Generic_GCC`) toolchain which expects GCC. Reviewed By: MaskRay, michaelplatings, #powerpc, nemanjai Differential Revision: https://reviews.llvm.org/D154357
The problem appeared as a segfault for case like this: ``` type t character(11), allocatable :: c end type character(12), alloctable :: x type(t) y y = t(x) ``` The frontend representes `y = t(x)` as `y=t(c=%SET_LENGTH(x,11_8))`. When 'x' is unallocated the hlfir.set_length lowering results in segfault. It could probably be handled in hlfir.set_length lowering by using NULL base for the hlfir.declare depending on the allocation status of 'x', but I am not sure if !hlfir.expr, in general, is supposed to represent an expression created from unallocated allocatable. I believe in Fortran that would mean referencing an unallocated allocatable, which is not allowed. I decided to special case `SET_LENGTH` in structure constructor, so that we use its 'x' operand as the RHS for the assign operation implying the isAllocatable check for cases when 'x' is allocatable. This requires setting keep_lhs_length_if_realloc flag for the assign operation. Note that when the component being intialized has deferred length the frontend does not produce `SET_LENGTH`. Differential Revision: https://reviews.llvm.org/D155151
…VMergeAndVOps [nfc] We have the SEW operand access repeating in all paths, common it up to make the code easier to read. This is a subset of Luke's D155063. I'm splitting pieces and landing them in the process of convincing myself all the individual transforms are in fact correct.
The early outlining pass was erasing target functions that need to be kept. It should only erase functions that contain target ops.
Very minor change, just making sure each step is obvious and easy to follow. This is a subset of Luke's D155063. I'm splitting pieces and landing them in the process of convincing myself all the individual transforms are in fact correct.
…for declare target This is an attempt at mimicing the method in which threadprivate handles the following type of variables: program main integer :: i !$omp declare target to(i) end Which essentially generates a GlobalOp for the variable (which would normally only be an alloca) when it's instantiated. The main difference is there is no operation generated within the function, instead the declare target attribute is appended later within handleDeclareTarget. Reviewers: kiranchandramohan Differential Revision: https://reviews.llvm.org/D152037
…:begin To fix expensive check builds that were failing when using MSVC's std::string_view::iterator::operator*, I added a few expressions like &*std::string_view::begin. @nico pointed out that this is literally the same thing and more clearly expressed as std::string_view::data. Link: llvm/llvm-project#63740 Reviewed By: #libc_abi, ldionne, philnik, MaskRay Differential Revision: https://reviews.llvm.org/D154876
…AndVOps [nfc] This is a subset of Luke's D155063. I'm splitting pieces and landing them in the process of convincing myself all the individual transforms are in fact correct. This particular change involves a slightly ugly bit of code to match the glue to the mask. I'm staging it this way as I ran into a bit of weirdness when commoning mask operands, and wanted to isolate the complexity.
ENABLE_X86_RELAX_RELOCATIONS has defaulted to on (c41a18c) for nearly 3 years. As a clean-up, remove overrides from some early adopters. Change OHOS to use true as agreed by the patch author D145227.
Setting initial offset of DIE to input DIE. This is to make "printf" debugging easier. Reviewed By: maksfb Differential Revision: https://reviews.llvm.org/D155031
…rseLt Also makes some minor consistency edits in the cuSparseLt wrapper lib. Reviewed By: Peiming, K-Wu Differential Revision: https://reviews.llvm.org/D155139
…rinsic. NFC The greediness of the operand matching regular expressions made the test pass even though an operand is missing.
…onstraint. GCC and existing codebases allow the use of integral values to be used with this constraint. A recent change D133914 in this area started causing asserts. Removing the assert is enough as the rest of the code works fine. rdar://109675485 Differential Revision: https://reviews.llvm.org/D155023
The DWO Unit DIE, doesn't have low_pc/high_pc, so we were printing this error for valid cases. Reviewed By: maksfb Differential Revision: https://reviews.llvm.org/D155032
There are cases in DWARF4 when Skeleton CU has ranges, but dwo CU doesn't. Bug was introduced in new DWARFRewriter where for DWARF4 it would fall through to DWARF5 case. Reviewed By: maksfb Differential Revision: https://reviews.llvm.org/D155033
This patch moves a few tests that were still using std::fprintf to using TEST_REQUIRE instead, which provides a single point to tweak for platforms that don't implement fprintf. As a fly-by fix, it also avoids including `time_utils.h` in filesystem_clock.cpp when it is not required, since that header makes some pretty large assumptions about the platform it is on. Differential Revision: https://reviews.llvm.org/D155019
This test has been failing on sanitizer-x86_64-linux-bootstrap-asan since it was commited. Removing this test while I work on reproducing this. Example: https://lab.llvm.org/buildbot/#/builders/168/builds/14579
Read ORC (oops rewind capability) info used for unwinding the stack by Linux Kernel. The info is stored in .orc_unwind and .orc_unwind_ip sections. There is also a related .orc_lookup section that is being populated by the kernel during runtime. Contents of the sections are sorted for quicker lookup by a post-link objtool. Unless we modify stack access instructions, we don't have to change ORC info attributed to instructions in the binary. However, we need to update instruction addresses and sort both sections based on the new layout. For pretty printing, we add "--print-orc" option that prints ORC info next to instructions in code dumps. Reviewed By: Amir Differential Revision: https://reviews.llvm.org/D154815
…ineVMergeAndVOps [nfc] This is a subset of Luke's D155063. I'm splitting pieces and landing them in the process of convincing myself all the individual transforms are in fact correct. The code structure here is overly verbose. I'm landing this staging change with the code structure exactly matching the non-masked case to make the following cleanup that commons this all obviously correct.
Add MetadataRewriter::postCFGInitializer(). Reviewed By: jobnoorman Differential Revision: https://reviews.llvm.org/D155153
|
@bader I think this is ready to merge. The cmath*.ll tests are flaky from what I've seen and everything else is passing. |
Do you mean sycl/test-e2e/DeviceLib/cmath*.cpp test? AFAIK, these are quite stable in the sycl branch, so I suppose failures are caused by the PR changes. I see you did changes on top of what we pull from the community. I'd like code owners to approve them before merging. @intel/dpcpp-spirv-reviewers, @intel/dpcpp-tools-reviewers, @intel/llvm-reviewers-runtime, @intel/dpcpp-esimd-reviewers, please, take a look at changes and let me if it's okay to merge. Here is the link with the diff for the latest 10 commits in this PR: https://github.com/intel/llvm/pull/10427/files/bbd88b0a0acc0c946f59e3a50c3788408e890dfc..HEAD. Please, review everything except lld changes. E.g. b2c98d5 - the test passes on Windows (see latest pre-commit results). We need to figure out why it doesn't work on Linux. @Chenyang-L, did you open a tracker for all the tests disabled in this PR? |
I opened a tracker for the lit tests disabled in this PR. I just noticed I included some Intel related changes that preserved some of the previous typed pointer code. If needed, those can be removed. |
|
For ESIMD, I have no idea why |
@sarnex, thanks for confirming. @intel/dpcpp-spirv-reviewers, @intel/dpcpp-tools-reviewers, @intel/llvm-reviewers-runtime, we need your confirmation too. Please, take a look at changes and let me if it's okay to merge. Details are in my previous comment - #10427 (comment). |
|
@intel/dpcpp-spirv-doc-reviewers @intel/dpcpp-tools-reviewers @intel/llvm-reviewers-runtime Can you please have a look? We need to get this merged ASAP, as we have HUGE backlog (sycl branch is 1.5 months behind main now), we need to start another pulldown soon. Thanks. |
Maybe it wasn't clear from the previous message, but we need to fix the status of the test on Windows. CI checks results are "fail" on Windows. @Chenyang-L, @jsji, please, make sure there are no regressions in CI checks except clang-format check. |
Did I interpret the result correctly? It is a unexpected pass? @Chenyang-L @bader ? Unexpectedly Passed Tests (1): |
jsji
force-pushed
the
llvmspirv_pulldown
branch
from
b2c98d5 to
3cdff00
Compare
I force-pushed to XFAIL this on Linux only. Let us see the test result. |
jsji
force-pushed
the
llvmspirv_pulldown
branch
from
3cdff00 to
3541c3e
Compare
|
Hmm.. Looks like we have some sort of CI env differences? 2 hours ago, CI is telling me, this test is XPASS: So I reverted it back to original state. Looking at the log: XPASS one is with only L0 runtime:
XFAILED one is with both OCL and L0 runtime:
But both are testing L0 runtime. |
|
Hmm, I am really confused by the CI here. I need some help to investigate what is really wrong here. @aelovikov-intel @bader Can either of you help here? Thanks. eg: I pushed a commit, and there are two LLVM and SPIRV-LLVM-Translator pulldown (WW29) The PR run was successful: https://github.com/intel/llvm/actions/runs/5815261173 |
Ping. |
|
I think we can drop this commit - fb840cd. It's not needed as we switched to opaque pointers by default. |
If we are going to do this, then we need to get rid of more:
Let me try to reorg the commit a little bit, but it might cause additional efforts to clean up these tests due to the outdated code. |
Conflicts: llvm-spirv/test/transcoding/multiple_user_semantic_on_struct.ll
…t.""" This reverts commit f146763.
Mark the current status -- the 4 failure are known failures -- 2 clang-format failures, 2 MacOS failures also seen in sycl. |
jsji
force-pushed
the
llvmspirv_pulldown
branch
from
c35f82b to
3963ab4
Compare
|
Force pushed to include only necessary changes -- discard all other opaque pointer lit changes first for now. Will cherry-pick some of them if we see failures in CI. |
For completeness, the two CI run is somehow a bug or know limitation in CI, one (push) is running against branch itself, the other one (PR) is running against sycl branch , and since we switched the opaque pointer in sycl branch, we end up getting opposite result here. Thanks @aelovikov-intel for helping to sorting out this, and follow up to get rid of duplicate post-commit CI run. |
So, looks like we DO need those commits like - fb840cd. 34b54f4 6a06f3e for the current stage of pulldown. We have two choice:
#2 will be better as it will get rid of these middle states of lit changes etc, |
|
Close this first, try to do a new pulldown PR with latest state instead. |
@jsji, I'm okay with option 1, if option 2 takes a lot of time. |
Thanks. Trying #2 in #10783 first. If we end up getting complications, then we can fall back to option 1 here. |
LLVM: llvm/llvm-project@1aecb0e
SPIRV-LLVM-Translator: KhronosGroup/SPIRV-LLVM-Translator@8dcd16c