Page MenuHomePhabricator

[GlobalISel] Change asserting conditions when initializing call site info
Needs RevisionPublic

Authored by matejam on Oct 1 2020, 8:58 AM.



IRTranslator's method runOnMachineFunction is used to translate from IR to MIR when using GlobalISel as an instruction selector.
When all IR instructions are translated, we move all the instructions from the current entry block to it's only successor, which leaves the current entry block empty and useless, we then delete it from the machine function.
The only issue is that the surviving entry block's number is greater by 1 than the minimal entry block number, which is fixed by changing the condition in MIRParserImpl::initializeCallSiteInfo, the idea is to have a special case when using GlobalISel. One way to figure out is GlobalISel the current selector is by not finding "bb.0" in the body of the machine function, the reason for that is beacuse GlobalISel will always start from bb.1, unlike SDAGISel or FastISel.

Diff Detail

Unit TestsFailed

1,100 mslinux > libarcher.parallel::parallel-nosuppression.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c

Event Timeline

matejam created this revision.Oct 1 2020, 8:58 AM
matejam requested review of this revision.Oct 1 2020, 8:58 AM
arsenm added inline comments.Oct 8 2020, 7:12 AM

I think treating this as some difference to tolerate is the wrong approach. There's no reason the block numbers should change based on the selector


A pure input MIR test in test/CodeGen/MIR would be preferable

djtodoro added inline comments.Oct 8 2020, 7:54 AM

Hi @arsenm, do you think the entry bb number should start from 0 in the case of GlobalIsel as well? If that is the case, is the D87902 right solution?

matejam added inline comments.Oct 8 2020, 8:13 AM

I agree with @djtodoro , entry bb number in GlobalISel start from 1 by default, unlike in other selectors where they start from 0. In case of a machine function which has only one entry bb it's number will be 1 and the MF.size() will also be 1 which will result in an error "if (BlockNum >= MF.size())".


It's an .ll test because in the translation process from IR to MIR (IRTranslator in case of using GlobalISel) the generated MIR code won't start from entry bb.0, that is what this test is checking.

arsenm added inline comments.Jan 12 2021, 12:08 PM

It's the same MIR. The numbering doesn't change. There's something pointlessly inconsistent if the block numbering ends up different

arsenm requested changes to this revision.Mar 30 2021, 6:07 PM

I think the block numbering is meaningless and shouldn't be any different between selectors

This revision now requires changes to proceed.Mar 30 2021, 6:07 PM