Page MenuHomePhabricator

[X86] Make ENDBR instruction a scheduling boundary
ClosedPublic

Authored by joaomoreira on Jul 29 2020, 9:04 AM.

Details

Summary

Instructions should not be scheduled across ENDBR instructions, as this would result in the ENDBR being displaced, breaking the parity needed for the Indirect Branch Tracking feature of CET.

Currently, the X86IndirectBranchTracking pass is later than the instruction scheduling in the pipeline, what causes the bug to be unnoticeable and very hard (if not unfeasible) to be triggered while compiling C files with the standard LLVM setup. Yet, for correctness and to prevent issues in future changes, the compiler should prevent the such scheduling.

Diff Detail

Unit TestsFailed

TimeTest
60 mslinux > Polly.Isl/CodeGen::invariant_load_escaping_second_scop.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen -polly-codegen-verify -polly-codegen -polly-invariant-load-hoisting=true -polly-process-unprofitable -S < /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/invariant_load_escaping_second_scop.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/invariant_load_escaping_second_scop.ll
130 mswindows > Clang.OpenMP::declare_variant_device_isa_codegen_1.c
Script: -- : 'RUN: at line 1'; c:\ws\w1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -verify -fopenmp -x c -triple x86_64-pc-windows-gnu -emit-llvm C:\ws\w1\llvm-project\premerge-checks\clang\test\OpenMP\declare_variant_device_isa_codegen_1.c -o - -fopenmp-version=50 | c:\ws\w1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w1\llvm-project\premerge-checks\clang\test\OpenMP\declare_variant_device_isa_codegen_1.c --check-prefix=GENERIC

Event Timeline

joaomoreira created this revision.Jul 29 2020, 9:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2020, 9:04 AM
joaomoreira requested review of this revision.Jul 29 2020, 9:04 AM

Tried to use the inline code editor - not sure if it worked that well.....

llvm/lib/Target/X86/X86InstrInfo.cpp
6681

Could this be simplified to use the base method as the fallback?

Fixes:

  • Applied suggestions made by @RKSimon,
  • Fixed "opcode" variable name capitalization.
lebedev.ri retitled this revision from Make ENDBR instruction a scheduling boundary to [X86] Make ENDBR instruction a scheduling boundary.Jul 31 2020, 3:54 AM

SGTM - @craig.topper are you happy with this to go in without a test case?

craig.topper accepted this revision.Aug 1 2020, 1:29 PM

LGTM. I think I'm ok with it. I suppose we could construct some synthetic MIR test case, but not sure how complex it would have to be to make the scheduler want to move the ENDBR.

This revision is now accepted and ready to land.Aug 1 2020, 1:29 PM
This revision was automatically updated to reflect the committed changes.