Page MenuHomePhabricator

[MBP] Prevent rotating a chain contains entry block
ClosedPublic

Authored by Carrot on Dec 8 2020, 1:32 PM.

Details

Summary

The entry block should always be the first BB in a function. So we should not rotate a chain contains the entry block.

Diff Detail

Unit TestsFailed

TimeTest
1,200 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp
60 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

Carrot created this revision.Dec 8 2020, 1:32 PM
Carrot requested review of this revision.Dec 8 2020, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 1:32 PM
xur accepted this revision.Dec 8 2020, 1:55 PM

Look reasonable to me.

This revision is now accepted and ready to land.Dec 8 2020, 1:55 PM

Can we add a test for this?

llvm/lib/CodeGen/MachineBlockPlacement.cpp
2392

Consider using the helper MachineBasicBlock::isEntryBlock here and above.

yes, a test case please :) Any performance numbers?

Carrot added a comment.Dec 9 2020, 4:18 PM

Can we add a test for this?

Sorry, no small test case for it.
It was triggered by https://reviews.llvm.org/D89088, when build firefox and chrome with LTO+PGO. This patch has been verified for them.

Carrot added a comment.Dec 9 2020, 4:24 PM

yes, a test case please :) Any performance numbers?

It doesn't have any performance impact.
If user doesn't notice anything, there is no such case for them.
If someone encountered such case, he should get a compiler crash in LiveDebugVariable pass.

It is possible to synthesize a test case that rotates the entry without the fix, no? The expected output is that the entry is not rotated (no need to reproduce crash).

It is possible to synthesize a test case that rotates the entry without the fix, no? The expected output is that the entry is not rotated (no need to reproduce crash).

The key is to generate an entry block and make the following condition in buildCFGChains() evaluated as false:

if (!TII->analyzeBranch(*BB, TBB, FBB, Cond) || !FI->canFallThrough())
  break;

It means the entry block must end with an unanalyzable branch and it must also fall through to following block, so the entry block will be pre-merged with the following block, the following block is a loop header, so later it can be rotated by rotateLoop.

I failed to construct such a test case after a whole day's experiments :(

davidxl accepted this revision.Dec 10 2020, 6:21 PM

thanks for working on the test -- good to add later if possible. LGTM after addressing snehasish's comment.

Carrot updated this revision to Diff 311308.Dec 11 2020, 1:41 PM
Carrot marked an inline comment as done.

Will commit this version.

This revision was automatically updated to reflect the committed changes.