This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][AArch64] Fix narrowScalar for G_ICMP
AbandonedPublic

Authored by Petar.Avramovic on Jul 22 2019, 5:57 AM.

Details

Diff Detail

Event Timeline

@brzycki
I don't have necessary toolchain for full build, but from error report it is most likely __int128 compare. I made small test that triggers same assert. Does it work now?

@dsanders
G_ICMP has s1 result(type0) by default. For AArch64 G_ICMP type0 has to be widened to s32.
Narrow Scalar for G_ICMP uses G_SELECT where condition(type1) is result of one of the new G_ICMP instructions. Only s1 for type1 is legal for G_SELECT.
During narrowScalar for G_ICMP we could make G_SELECT with same type1 as a result(type0) for given G_ICMP but then again G_SELECT is only legal for s1(not for s32).
Here I put widen scalar rule for type0 in G_ICMP after clampScalar for type1 since G_ICMP and G_SELECT have different legalization strategies for operand with default s1 type.
Added a test to check that type mismatch assert does not trigger anymore.

Here is the llvm-ir test

define i1 @test_icmp_s128(i128* %x, i128* %y) {
entry:
  %0 = load i128, i128* %x
  %1 = load i128, i128* %y
  %cmp = icmp slt i128 %0, %1
  ret i1 %cmp
}

Hello @Petar.Avramovic , I have verified this patch on a recent sha 510e6fadaae95a41bee0eb99d84a146efc612365 does not crash clang when compiling the libcxx shared library. Thank you for identifying the source of the crash.

I don't feel comfortable enough reviewing this change for all Aarch64 targets and recommend @evandro , @SjoerdMeijer, @rengolin, and/or @t.p.northover examine the change to lib/Target/AArch64/AArch64LegalizerInfo.cpp and its potential implications.

aemerson requested changes to this revision.Jul 24 2019, 1:47 PM
aemerson added a subscriber: aemerson.

I don't believe this is the right fix as the legalizer shouldn't really be hard coding the type of G_SELECT operands there. I fixed it by using the existing type of the G_ICMP in r366943.

This revision now requires changes to proceed.Jul 24 2019, 1:47 PM

Thank you @aemerson , the fix in rL366943 seems to have prevented crashes when compiling libcxx for aarch64. I successfully compiled a nightly with sha f181dd99cf1c4e .