Page MenuHomePhabricator

[ValueTracking] Recognize more integer abs idioms
AbandonedPublic

Authored by frasercrmck on Oct 22 2020, 6:19 AM.

Details

Summary

This patch supports additional integer abs idioms where the comparison
is performed in a wider type than the abs result, i.e. where select's
comparison operands are sign-extended versions of the select's
true/false operands:

  %neg = sub i16 0, %a
  %a.prom = sext i16 %a to i32
  %abs.cond = icmp sge i32 %a.prom, 0
  %abs = select i1 %abs.cond, i16 %a, i16 %neg

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

frasercrmck created this revision.Oct 22 2020, 6:19 AM
frasercrmck requested review of this revision.Oct 22 2020, 6:19 AM
nikic added a comment.Oct 22 2020, 1:58 PM

This pattern already gets canonicalized by InstCombine (and there are no multi-use problems): https://llvm.godbolt.org/z/od9oq4 As such, it shouldn't be reaching the backend in this form.

This pattern already gets canonicalized by InstCombine (and there are no multi-use problems): https://llvm.godbolt.org/z/od9oq4 As such, it shouldn't be reaching the backend in this form.

That's a good point, thanks. Sadly I don't have access to my motivating example as I've lost access to the code. I suppose it's possible that there was a pass between InstCombine and the SelectionDAG lowering which undid this canonicalization, but it'll be hard to find.

Out of curiosity, what do you mean by multi-use problems? Profitability or correctness?

shchenz added a comment.EditedOct 27 2020, 1:18 AM

I suppose it's possible that there was a pass between InstCombine and the SelectionDAG lowering which undid this canonicalization, but it'll be hard to find.

The issue here is there is no instruction combine/simplify pass in llc pipeline which eliminates the %a.prom = sext i16 %a to i32 before we canonicalize abs.

In opt pipeline, InstCombine will eliminate the sext first and then can recognize it as a abs.

If we compile with clang which contain inst combine pass, we will get expected abs instruction.

Please can you explain the bigger problem here?
I'm not sure why the solution isn't: "just run the middle-end optimization passes, or don't expect good back-end codegen".

Please can you explain the bigger problem here?
I'm not sure why the solution isn't: "just run the middle-end optimization passes, or don't expect good back-end codegen".

The original motivation was that when we moved from LLVM7 to LLVM9 we observed a degradation in the recognition of abs idioms (using clang, so including InstCombine @shchenz). I tracked it to the time at which abs idioms were moved out of DAGCombine and into the initial lowering from IR, where I found a potential for more patterns. It's possible something happened to our code in between InstCombine and ISel as our target had support for i8/i16 but only when extended/truncated to/from i32: exts & truncs were common and it was generally awkward in LLVM. Sadly this is all hypothetical since I've lost access to the code so I can't get at the full reproduction details.

I put this patch up anyway, since I thought that the ValueTracking pattern analysis had a function outside of the specific position(s) in the pipeline it's currently run. I reasoned that you might want to match this select pattern before InstCombine, for instance. If that's not ValueTracking's agreed-upon contract (which I'd understand since then its scope becomes much larger: it has to settle on some form of canonicalization) then your solution is the one we should take. It sounds to me like a question of determining which pass is responsible for what.

lebedev.ri requested changes to this revision.Oct 27 2020, 4:06 AM

Since as @nikic said this pattern is canonicalized to the intrinsic already, i don't think this should be needed.

This revision now requires changes to proceed.Oct 27 2020, 4:06 AM
frasercrmck abandoned this revision.Oct 27 2020, 4:26 AM