Page MenuHomePhabricator

[GlobalISel] Fold xor(cmp(pred, _, _), 1) -> cmp(inverse(pred), _, _)
ClosedPublic

Authored by aemerson on Aug 21 2020, 3:56 PM.

Details

Summary

This is needed for an upcoming change to how we translate conditional branches which might generate these.

Diff Detail

Event Timeline

aemerson created this revision.Aug 21 2020, 3:56 PM
aemerson requested review of this revision.Aug 21 2020, 3:56 PM
arsenm added inline comments.Aug 21 2020, 4:17 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2186–2192

Can generalize for not-i1 by checking the target boolean contents

aemerson updated this revision to Diff 287193.Aug 22 2020, 9:10 AM

Use getBooleanContents and update AMDGPU tests.

foad added a subscriber: foad.Aug 23 2020, 12:16 AM
aemerson updated this revision to Diff 287709.Aug 25 2020, 10:38 AM

Check for multiple uses of the cmp.

arsenm added inline comments.Wed, Aug 26, 4:47 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2192

Swap the order of these checks?

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir
137

Test with vectors?

foad added a comment.Thu, Aug 27, 3:25 AM

One of the globalisel code quality regressions vs selectiondag that I'm tracking comes from code like this:

%40:_(s1) = G_ICMP intpred(uge), %35:_(s32), %37:_
%41:_(s1) = G_ICMP intpred(uge), %36:_(s32), %38:_
%42:_(s1) = G_OR %40:_, %41:_
%43:_(s1) = G_CONSTANT i1 true
%44:sreg_64_xexec(s1) = G_XOR %42:_, %43:_
%46:sreg_64_xexec(s64) = SI_IF %44:sreg_64_xexec(s1), %bb.3, implicit-def $exec, implicit-def $scc, implicit $exec

So... is there any chance of extending this patch to be able to invert a whole tree of comparisons combined with ANDs and ORs?

foad added a comment.Thu, Aug 27, 6:44 AM

So... is there any chance of extending this patch to be able to invert a whole tree of comparisons combined with ANDs and ORs?

I've had a go at this in D86709.

aemerson updated this revision to Diff 288461.Thu, Aug 27, 2:45 PM

Add support for vectors.

arsenm added inline comments.Thu, Aug 27, 3:09 PM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
732

One of these parameters is isVector, which is getting lost here

aemerson updated this revision to Diff 288475.Thu, Aug 27, 3:54 PM
arsenm added inline comments.Fri, Aug 28, 11:56 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2239–2251

I think this isConstTrue logic should be merged into a separate function (at least the getScalarSizeInBits() == 1 case and check TLI for the extended version should be together)

2258

I guess technically TargetBooleanContents also depends on isFloat in the fcmp case

aemerson updated this revision to Diff 288897.Mon, Aug 31, 12:38 AM

Simplify and refactor code with FP case.

Personally I can't see anything wrong with this at this point.

@arsenm?

arsenm accepted this revision.Tue, Sep 1, 9:49 AM
This revision is now accepted and ready to land.Tue, Sep 1, 9:49 AM
This revision was landed with ongoing or failed builds.Tue, Sep 1, 10:57 AM
This revision was automatically updated to reflect the committed changes.

FYI, I reverted this (and D86413 which depends on it) in 8693ddc74371dedc742c9f3d3e4eda1da72c13ea to keep the build green, as it was causing surprising crashes when running ninja check-llvm-codegen-aarch64-globalisel in release builds.

In case it goes away, the buildbot error is:

FAIL: LLVM :: CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir (33345 of 67618)
******************** TEST 'LLVM :: CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir' FAILED ********************
Script:
--
: 'RUN: at line 2';   /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc -mtriple aarch64-apple-ios  -run-pass=aarch64-prelegalizer-combiner --aarch64prelegalizercombinerhelper-only-enable-rule="not_cmp_fold" /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir -o - -verify-machineinstrs | /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/FileCheck /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir
--
Exit Code: 2

Command Output (stderr):
--
LLVM ERROR: Invalid rule identifier
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc -mtriple aarch64-apple-ios -run-pass=aarch64-prelegalizer-combiner --aarch64prelegalizercombinerhelper-only-enable-rule=not_cmp_fold /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir -o - -verify-machineinstrs 
1.	Running pass 'Function Pass Manager' on module '/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir'.
2.	Running pass 'AArch64PreLegalizerCombiner' on function '@icmp'
 #0 0x00005651667a183c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x25cc83c)
 #1 0x000056516679f694 llvm::sys::RunSignalHandlers() (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x25ca694)
 #2 0x000056516679f7e3 SignalHandler(int) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x25ca7e3)
 #3 0x00007f71603c58a0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x128a0)
 #4 0x00007f715f293f47 raise (/lib/x86_64-linux-gnu/libc.so.6+0x3ef47)
 #5 0x00007f715f2958b1 abort (/lib/x86_64-linux-gnu/libc.so.6+0x408b1)
 #6 0x000056516671a1d6 llvm::report_fatal_error(llvm::Twine const&, bool) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x25451d6)
 #7 0x000056516671a2f8 (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x25452f8)
 #8 0x0000565164a62751 (anonymous namespace)::AArch64PreLegalizerCombiner::runOnMachineFunction(llvm::MachineFunction&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x88d751)
 #9 0x0000565165d08b78 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x1b33b78)
#10 0x00005651660d9af7 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x1f04af7)
#11 0x00005651660da211 llvm::FPPassManager::runOnModule(llvm::Module&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x1f05211)
#12 0x00005651660d8e6f llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x1f03e6f)
#13 0x000056516491a6e7 compileModule(char**, llvm::LLVMContext&) (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x7456e7)
#14 0x0000565164871cf7 main (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x69ccf7)
#15 0x00007f715f276b97 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b97)
#16 0x000056516491380a _start (/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/llc+0x73e80a)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/build/stage1/bin/FileCheck /home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir

Thanks, it seems the test doesn't work without asserts enabled. I've re-committed it fixed in 520ab710fb6f9829b4e70fda1dcc91ed4f614d0a

Thanks!

Usually, tests that are marked # REQUIRES: asserts are done so because they make use of some code that is only available behind NDEBUG. However, in this case, it's outright crashing on certain inputs when not in debug mode. At a first glance, that doesn't seem like a correct fix to me -- after all, the crashing bug is still there -- but I'm not familiar with this code, so I'm probably missing something?

Thanks!

Usually, tests that are marked # REQUIRES: asserts are done so because they make use of some code that is only available behind NDEBUG. However, in this case, it's outright crashing on certain inputs when not in debug mode. At a first glance, that doesn't seem like a correct fix to me -- after all, the crashing bug is still there -- but I'm not familiar with this code, so I'm probably missing something?

There's no bug in the actual patch, the crash is because the testing flag --aarch64prelegalizercombinerhelper-only-enable-rule doesn't work without assertions enabled. It's not an elegant failure mode, but nothing to worry I think.

Ok. Thanks for confirming!