Page MenuHomePhabricator

[MC] Adding MCContext parameter to MCAsmBackend::relaxInstruction
Needs ReviewPublic

Authored by DavidFerencSzabo on Jan 25 2021, 9:37 AM.

Details

Reviewers
andreadb
MaskRay
Summary

The motivation is to be able to relax out of range conditional branches when the input is assembly. The existing pass not works at this level. To achieve this the conditional branch must be relaxed into an instructions pair, which then put into a BUNDLE. The change allows targets to create MCInst objects using MCContext object inside the relaxation function.

Diff Detail

Unit TestsFailed

TimeTest
2,670 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /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/races/lock-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c

Event Timeline

DavidFerencSzabo requested review of this revision.Jan 25 2021, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 9:37 AM

which then put into a BUNDLE.

Does the BUNDLE here mean .bundle_{align_mode,lock,unlock}? (something I want to get rid of (D78441))

DavidFerencSzabo added a comment.EditedJan 25 2021, 10:58 AM

@MaskRay No. Its a Machine Instruction Bundle. (https://lists.llvm.org/pipermail/llvm-dev/2011-December/045851.html).

But reading this I stumbled upon this line

There is no need to add the equivalent of MI bundle to MCInst. A MI bundle should be concatenated into a single MCInst by storing opcodes as integer operands. e.g.

This whole change might be unnecessary and the idiomatic way to do this is concatenation?

@MaskRay No. Its a Machine Instruction Bundle. (https://lists.llvm.org/pipermail/llvm-dev/2011-December/045851.html).

But reading this I stumbled upon this line

OK. Currently MCRelaxableFragment encodes just one MCInst. Do you intend to loose the requirement?

OK. Currently MCRelaxableFragment encodes just one MCInst. Do you intend to loose the requirement?

Well, not sure. What we much rather would like to know is what the intended way to implement this correctly is. Apart from Hexagon, I am not aware of any other target that implements this relaxation. Hexagon does use BUNDLE, but in order to get the required MCContext, they have to resort to some hacky implementation leveraging fixupNeedsRelaxation IIRC.

We could implement this using a pseudo instruction, that we then expand in our target's MCCodeEmitter. But we thought that maybe there is no context available in relaxInstructions, because no other target implements this yet, so instead of working around this limitation using a pseudo, we should simply make the context available.

We really are unsure what the intended way should be, so we would definitely like some feedback here.

@MaskRay I added you as a reviewer. Also I would like to ask you to add more people if you know any, who might should have an eye on this.

bcain added a subscriber: bcain.Tue, Feb 2, 7:06 AM