[Bug]: NDK27 clang hangs compiling google/skia

tiagomacarios commented 2 months ago

Description

Clang hangs indefinitely when compiling version m101 of google/skia. Repro attached - skia_hang.zip

I tried to repro the issue with clang version 18.1.0rc and it did not repro.

PS: Would it be possible to add pdbs to the release? This would help us try to find workarounds and/or provide you more info about the issue.

Upstream bug

No response

Commit to cherry-pick

No response

Affected versions

r27

Canary version

No response

Host OS

Windows

Host OS version

Windows 10/11

Affected ABIs

arm64-v8a

DanAlbert commented 2 months ago

PS: Would it be possible to add pdbs to the release? This would help us try to find workarounds and/or provide you more info about the issue.

File a separate FR, please. GitHub doesn't allow me to manage half an issue.

DanAlbert commented 2 months ago

This probably won't make it into r27b btw, but I'm triaging it there for now in case we have a fix before that goes to QA (supposed to have happened already but we're blocked on some infra issues, so maybe). If not, we'll aim for r27c.

I don't suppose you know if this is a regression from r26? We'll try to find out ourselves, but if you already know that saves us some time.

tiagomacarios commented 2 months ago

I don't suppose you know if this is a regression from r26? We'll try to find out ourselves, but if you already know that saves us some time.

We are currently on r26b and it compiles fine.

DanAlbert commented 2 months ago

Thanks for confirming 👍

pirama-arumuga-nainar commented 2 months ago

I can reproduce the timeout. It doesn't reproduce with clang-r530567 (slated tentatively for r28 - in case @DanAlbert was wondering). Bisecting to find the fix.

Given the blockers in generating new clang prebuilts for r27b, we could potentially include the fix in r27b.

pirama-arumuga-nainar commented 2 months ago

070848c17c2944afa494d42d3ad42929f3379842 is the first new commit            
commit 070848c17c2944afa494d42d3ad42929f3379842                                                          
Author: Nikita Popov <npopov@redhat.com>                                                                                                                                                                           
Date:   Tue Feb 13 09:29:56 2024 +0100                                                                   

    [AArch64][GISel] Don't pointlessly lower G_TRUNC (#81479)        

    If we have something like G_TRUNC from v2s32 to v2s16, then lowering
    this to a concat of two G_TRUNC s32 to s16 followed by G_TRUNC from
    v2s16 to v2s8 does not bring us any closer to legality. In fact, the
    first part of that is a G_BUILD_VECTOR whose legalization will produce a                                                                                                                                       
    new G_TRUNC from v2s32 to v2s16, and both G_TRUNCs will then get
    combined to the original, causing a legalization cycle. 

    Make the lowering condition more precise, by requiring that the original              
    vector is >128 bits, which is I believe the only case where this
    specific splitting approach is useful.

    Note that this doesn't actually produce a legal result (the alwaysLegal
    is a lie, as before), but it will cause a proper globalisel abort
    instead of an infinite legalization loop.

    Fixes https://github.com/llvm/llvm-project/issues/81244.

 .../Target/AArch64/GISel/AArch64LegalizerInfo.cpp  |  5 ++---
 .../CodeGen/AArch64/GlobalISel/legalize-xtn.mir    | 24 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

pirama-arumuga-nainar commented 2 months ago

r.android.com/3238718 cherry-picks the fix. Uploading new prebuilts shortly.

android / ndk