Page MenuHomePhabricator

[InstCombine] Simplify PHI node whose type and type of its cond differ
Changes PlannedPublic

Authored by hkmatsumoto on Apr 1 2022, 8:43 AM.



Currently LLVM optimizes the code below:

define i8 @foo(i8 %cond) {
  switch i8 %cond, label %default [
  i8 -1, label %sw.-1
  i8 0, label %sw.0
  i8 1, label %sw.1


  br label %merge

  br label %merge

  br label %merge

  %ret = phi i8 [ 1, %sw.1 ], [ 0, %sw.0 ], [ -1, %sw.-1 ]
  ret i8 %ret


define i8 @foo(i8 %cond) {
  ret i8 %cond

But once we make @foo return larger types, say i32 it no longer
optimizes. This patch covers such cases.


Diff Detail

Unit TestsFailed

320 msx64 debian > BOLT.X86::asm-dump.c
Script: -- : 'RUN: at line 7'; /usr/bin/clang -fPIC /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/asm-dump.c -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/X86/Output/asm-dump.c.tmp.exe -Wl,-q
340 msx64 debian > BOLT.X86::exceptions-args.test
Script: -- : 'RUN: at line 4'; /usr/bin/clang --driver-mode=g++ -no-pie -gdwarf-4 /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/Inputs/exc_args.s -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/X86/Output/exceptions-args.test.tmp
330 msx64 debian > BOLT.X86::inline-debug-info.test
Script: -- : 'RUN: at line 6'; /usr/bin/clang -no-pie -gdwarf-4 -O1 -g /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/Inputs/inline-main.c /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/Inputs/inline-foo.c -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/X86/Output/inline-debug-info.test.tmp.exe -Wl,-q
60 msx64 debian > BOLT.X86::plt-sec-8-byte.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/yaml2obj /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/Inputs/plt-sec-8-byte.yaml &> /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/X86/Output/plt-sec-8-byte.test.tmp.exe
90 msx64 debian > BOLT.X86::plt-sec.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/yaml2obj /var/lib/buildkite-agent/builds/llvm-project/bolt/test/X86/Inputs/plt-sec.yaml &> /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/X86/Output/plt-sec.test.tmp.exe
View Full Test Results (12 Failed)

Event Timeline

hkmatsumoto created this revision.Apr 1 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 8:43 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
hkmatsumoto requested review of this revision.Apr 1 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 8:43 AM
hkmatsumoto added inline comments.Apr 1 2022, 8:53 AM

Since we now have to store integers of heterogeneous types (e.g. i8 1 vs i32 1), we have to align integer types to the biggest one when accessing to SmallDenseMap. APInt has an API for aligning integers types (sextOrSelf) but not for ConstantInt AFAIK, that's why the type had to be changed.

hkmatsumoto edited the summary of this revision. (Show Details)Apr 1 2022, 8:55 AM
nikic added inline comments.Apr 5 2022, 8:28 AM

All inputs of a phi will have the same size (which is the same as the phi result type width), so computing a max in a loop doesn't make sense to me.


I don't like the fact that this privileges sext. The same optimization is also possible with zext (depending on values used) and in some cases both are possible -- in which case our policy is to prefer zext over sext. I think we should be adding zext and sext support at the same time.

Also, we need a test with switch width wider than phi width rather than the other way around. I think you'll currently assert in that case.


Please drop the sroa run here and only use the phi representation. If you want to do an end-to-end tests, add it to PhaseOrdering (e.g. llvm/test/Transforms/PhaseOrdering/simplifycfg-switch-lowering-vs-correlatedpropagation.ll is related).

hkmatsumoto planned changes to this revision.Apr 25 2022, 8:58 PM

Sorry, currently I don't have the bandwidth to push this forward due to the university assignments. If someone's interested in taking this over, you can do so.