Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/include/clang/Driver/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -4941,6 +4941,7 @@ def mload_store_pairs : Flag<["-"], "mload-store-pairs">, Group<m_riscv_Features
def mno_load_store_pairs : Flag<["-"], "mno-load-store-pairs">, Group<m_riscv_Features_Group>;
def mccmov : Flag<["-"], "mccmov">, Group<m_riscv_Features_Group>;
def mno_ccmov : Flag<["-"], "mno-ccmov">, Group<m_riscv_Features_Group>;
def mremove_back_to_back_branches : Flag<["-"], "mremove_back_to_back_branches">, Group<m_riscv_Features_Group>;
let Flags = [TargetSpecific] in {
def menable_experimental_extensions : Flag<["-"], "menable-experimental-extensions">, Group<m_Group>,
HelpText<"Enable use of experimental RISC-V extensions.">;
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Driver/ToolChains/Clang.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2178,6 +2178,10 @@ void Clang::AddRISCVTargetArgs(const ArgList &Args,
CmdArgs.push_back("-riscv-ccmov=0");
}
}
if (Args.getLastArg(options::OPT_mremove_back_to_back_branches)) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-riscv-remove-back-to-back-branches=1");
}
// Handle -mrvv-vector-bits=<bits>
if (Arg *A = Args.getLastArg(options::OPT_mrvv_vector_bits_EQ)) {
StringRef Val = A->getValue();
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Target/RISCV/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ add_llvm_target(RISCVCodeGen
RISCVMoveMerger.cpp
RISCVPushPopOptimizer.cpp
RISCVRegisterInfo.cpp
RISCVRemoveBackToBackBranches.cpp
RISCVSubtarget.cpp
RISCVTargetMachine.cpp
RISCVTargetObjectFile.cpp
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/RISCV/RISCV.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ createRISCVInstructionSelector(const RISCVTargetMachine &,
const RISCVRegisterBankInfo &);
void initializeRISCVDAGToDAGISelLegacyPass(PassRegistry &);

FunctionPass *createRISCVRemoveBackToBackBranches();
void initializeRISCVRemoveBackToBackBranchesPass(PassRegistry &);

FunctionPass *createRISCVPostLegalizerCombiner();
void initializeRISCVPostLegalizerCombinerPass(PassRegistry &);

Expand Down
158 changes: 158 additions & 0 deletions llvm/lib/Target/RISCV/RISCVRemoveBackToBackBranches.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
//===----------------------- RISCVRemoveBackToBackBranches.cpp ------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "RISCV.h"
#include "RISCVInstrInfo.h"
#include "RISCVSubtarget.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineFunction.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
#include "llvm/CodeGen/MachineInstr.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
#include "llvm/Target/TargetMachine.h"

using namespace llvm;

#define DEBUG_TYPE "riscv-remove-back-to-back-branches"

STATISTIC(NumInsertedAligments, "Number of aligments set");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: aligments => alignments


namespace {

// According to the MIPS specification, there shouldn't be two conditional
// branches in the same 8-byte aligned region of code.
Comment on lines +29 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe this Pass in a bit more depth? The RISC-V specification doesn't have this restriction as far as I understand - is this a performance optimisation for your core, or is it required for your core to work correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I’ll describe it in more detail. To clarify, this is not a general RISC-V specification but rather a MIPS-specific requirement necessary for our core to function correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a requirement to function correctly then your core is not a RISC-V core...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to explain in detail why it was needed :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will move this into a separate PR commit.
After reviewing what I wrote earlier, I now understand what 'work correctly' means in this context. Functionally, the core will operate correctly, but this change could impact performance. Specifically, if two branches share the same predictor index, this feature would come into play. However, this change primarily addresses an older RTL issue, so it is not strictly necessary and can be omitted or kept as off by default. Apologies for any confusion caused.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's reassuring and rather more palatable

constexpr unsigned NumberOfBytesOfCodeRegion = 8;

class RISCVRemoveBackToBackBranches : public MachineFunctionPass {
public:
static char ID;

RISCVRemoveBackToBackBranches() : MachineFunctionPass(ID) {
initializeRISCVRemoveBackToBackBranchesPass(
*PassRegistry::getPassRegistry());
}

StringRef getPassName() const override {
return "RISCV Remove Back To Back Branches Pass";
}

bool runOnMachineFunction(MachineFunction &F) override;

MachineFunctionProperties getRequiredProperties() const override {
return MachineFunctionProperties().set(
MachineFunctionProperties::Property::NoVRegs);
}

private:
const RISCVSubtarget *STI;
const RISCVInstrInfo *TII;
};

} // end of anonymous namespace

char RISCVRemoveBackToBackBranches::ID = 0;

INITIALIZE_PASS(RISCVRemoveBackToBackBranches, DEBUG_TYPE,
"Fix hazards by removing back to back branches", false, false)

/// Returns a pass that clears pipeline hazards.
FunctionPass *llvm::createRISCVRemoveBackToBackBranches() {
return new RISCVRemoveBackToBackBranches();
}

static bool CheckCompressedISA(MachineBasicBlock *MBB,
const RISCVInstrInfo *TII) {
unsigned SizeInBytes = 0;
for (auto &I : *MBB) {
// Skip some 0-sized meta instrucitons, such as debug ones.
if (!TII->getInstSizeInBytes(I))
continue;

SizeInBytes += TII->getInstSizeInBytes(I);

// This means that there is something other than the conditional branch
// here.
if (!I.isConditionalBranch())
continue;

// If it is a conditional branch, make sure it is the last one
// in this MBB and the cumulative size in bytes of other instructions in the
// block is <= 6 (since there potentially could be space for the two
// branches in the same 8-byte aligned code region, when compressed version
// of the instructions (16-bit size) is being used).
if (&I == &*MBB->getLastNonDebugInstr()) {
if (SizeInBytes <= 6)
return true;
return false;
}
}

return false;
}

static bool CheckNonCompressedISA(MachineBasicBlock *MBB,
const RISCVInstrInfo *TII) {
for (auto &I : *MBB) {
// Skip some 0-sized meta instrucitons, such as debug ones.
if (!TII->getInstSizeInBytes(I))
continue;

// This means that there is something other than the conditional branch
// here.
if (!I.isConditionalBranch())
return false;

// If it is a conditional branch, make sure it is the last one
// in this MBB.
if (&I == &*MBB->getLastNonDebugInstr())
return true;
return false;
}
return false;
}

bool RISCVRemoveBackToBackBranches::runOnMachineFunction(MachineFunction &MF) {
STI = &static_cast<const RISCVSubtarget &>(MF.getSubtarget());
TII = static_cast<const RISCVInstrInfo *>(STI->getInstrInfo());

if (!STI->shouldRemoveBackToBackBranches()) {
LLVM_DEBUG(llvm::dbgs()
<< "Ignoring RISCV Remove Back To Back Branches Pass\n");
return false;
}

bool Changed = false;
for (auto &MBB : MF) {
auto BBTerminator = MBB.getFirstTerminator();
// If it is not a conditional branch, we are not interested.
if (BBTerminator == MBB.end() ||
&*BBTerminator != &*MBB.getLastNonDebugInstr() ||
!BBTerminator->isConditionalBranch())
continue;

for (auto &Successor : MBB.successors()) {
// Set up aligment in order to avoid hazards. No 2 conditional branches
// should be in the same 8-byte aligned region of code. Similar to MIPS
// forbidden slots problem. We may want to insert a NOP only, but we
// need to think of Compressed ISA, so it is more safe to just set up
// aligment to the successor block if it meets requirements.
bool ShouldSetAligment = STI->getFeatureBits()[RISCV::FeatureStdExtC]
? CheckCompressedISA(Successor, TII)
: CheckNonCompressedISA(Successor, TII);
if (ShouldSetAligment) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the successor of a BB is itself, you probably don't want to add alignment

Successor->setAlignment(Align(NumberOfBytesOfCodeRegion));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the successor BB code precedes the BB code, will alignment really fix the issue?

For example consider the following BBs sequence:

Some.BB:
jump to cond.BB
True.BB:
...
False.BB:
...
cond.BB:
branch to True.BB or False.BB:

Changed = true;
++NumInsertedAligments;
}
}
}

return Changed;
}
9 changes: 9 additions & 0 deletions llvm/lib/Target/RISCV/RISCVSubtarget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ static cl::opt<bool> UseCCMovInsn("riscv-ccmov",
cl::desc("RISCV: Use 'ccmov' instruction"),
cl::init(true), cl::Hidden);

static cl::opt<bool> RISCVRemoveBackToBackBranches(
"riscv-remove-back-to-back-branches",
cl::desc("RISCV: Insert nops to clear pipeline hazards."), cl::init(false),
cl::Hidden);

void RISCVSubtarget::anchor() {}

RISCVSubtarget &
Expand Down Expand Up @@ -233,3 +238,7 @@ bool RISCVSubtarget::useLoadStorePairs() const {
bool RISCVSubtarget::useCCMovInsn() const {
return UseCCMovInsn && HasMIPSCMov;
}

bool RISCVSubtarget::shouldRemoveBackToBackBranches() const {
return RISCVRemoveBackToBackBranches && hasFeature(RISCV::TuneMIPSP8700);
}
5 changes: 2 additions & 3 deletions llvm/lib/Target/RISCV/RISCVSubtarget.h
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,8 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
MVT getXLenVT() const {
return is64Bit() ? MVT::i64 : MVT::i32;
}
unsigned getXLen() const {
return is64Bit() ? 64 : 32;
}
unsigned getXLen() const { return is64Bit() ? 64 : 32; }
bool shouldRemoveBackToBackBranches() const;
bool useLoadStorePairs() const;
bool useCCMovInsn() const;
unsigned getFLen() const {
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -579,13 +579,14 @@ void RISCVPassConfig::addPreEmitPass() {
if (TM->getOptLevel() >= CodeGenOptLevel::Default &&
EnableRISCVCopyPropagation)
addPass(createMachineCopyPropagationPass(true));
addPass(&BranchRelaxationPassID);
addPass(createRISCVMakeCompressibleOptPass());

// LoadStoreOptimizer creates bundles for load-store bonding.
addPass(createUnpackMachineBundles([](const MachineFunction &MF) {
return MF.getSubtarget<RISCVSubtarget>().useLoadStorePairs();
}));
addPass(&BranchRelaxationPassID);
addPass(createRISCVRemoveBackToBackBranches());
}

void RISCVPassConfig::addPreEmitPass2() {
Expand Down
108 changes: 108 additions & 0 deletions llvm/test/CodeGen/MIR/RISCV/riscv-remove-back-to-back-branches.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# RUN: llc -mtriple=riscv64 -mattr=-c -riscv-remove-back-to-back-branches=1 -o - %s | FileCheck %s

# CHECK: %bb.0:
# CHECK: blez
# CHECK: .p2align 3
# CHECK: %bb.1:
# CHECK: blez

--- |
; ModuleID = 'hazaard.c'
source_filename = "hazaard.c"
target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
target triple = "riscv64-unknown-linux-gnu"

; Function Attrs: nounwind optsize
define dso_local void @test(i32 noundef signext %a, i32 noundef signext %b) local_unnamed_addr #0 {
entry:
%cmp = icmp sgt i32 %a, 0
br i1 %cmp, label %if.then, label %if.end3

if.then: ; preds = %entry
%cmp1 = icmp slt i32 %b, 1
br i1 %cmp1, label %if.then2, label %if.end3

if.then2: ; preds = %if.then
tail call void asm sideeffect "nop", ""() #1, !srcloc !4
ret void

if.end3: ; preds = %if.then, %entry
ret void
}

attributes #0 = { nounwind optsize "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="mips-p8700" "target-features"="+64bit,+a,+c,+d,+f,+m" }
attributes #1 = { nounwind }

!llvm.module.flags = !{!0, !1, !2}
!llvm.ident = !{!3}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 1, !"target-abi", !"lp64d"}
!2 = !{i32 1, !"SmallDataLimit", i32 8}
!3 = !{!"clang version 14.0.0 ([email protected]:MIPS/llvm.git ae54cf4034587fab977092097c9772c7a275ddc8)"}
!4 = !{i64 88}

...
---
name: test
alignment: 2
exposesReturnsTwice: false
legalized: false
regBankSelected: false
selected: false
failedISel: false
tracksRegLiveness: true
hasWinCFI: false
failsVerification: false
tracksDebugUserValues: true
registers: []
liveins:
- { reg: '$x10', virtual-reg: '' }
- { reg: '$x11', virtual-reg: '' }
frameInfo:
isFrameAddressTaken: false
isReturnAddressTaken: false
hasStackMap: false
hasPatchPoint: false
stackSize: 0
offsetAdjustment: 0
maxAlignment: 1
adjustsStack: false
hasCalls: false
stackProtector: ''
maxCallFrameSize: 0
cvBytesOfCalleeSavedRegisters: 0
hasOpaqueSPAdjustment: false
hasVAStart: false
hasMustTailInVarArgFunc: false
hasTailCall: false
localFrameSize: 0
savePoint: ''
restorePoint: ''
fixedStack: []
stack: []
callSites: []
debugValueSubstitutions: []
constants: []
machineFunctionInfo: {}
body: |
bb.0.entry:
successors: %bb.1(0x50000000), %bb.2(0x30000000)
liveins: $x10, $x11

BGE $x0, killed renamable $x10, %bb.2

bb.1.if.then:
successors: %bb.3(0x30000000), %bb.2(0x50000000)
liveins: $x11

BGE $x0, killed renamable $x11, %bb.3

bb.2.if.end3:
PseudoRET

bb.3.if.then2:
INLINEASM &nop, 1 /* sideeffect attdialect */, !4
PseudoRET

...
3 changes: 2 additions & 1 deletion llvm/test/CodeGen/RISCV/O0-pipeline.ll
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@
; CHECK-NEXT: Insert fentry calls
; CHECK-NEXT: Insert XRay ops
; CHECK-NEXT: Implement the 'patchable-function' attribute
; CHECK-NEXT: Branch relaxation pass
; CHECK-NEXT: RISC-V Make Compressible
; CHECK-NEXT: Unpack machine instruction bundles
; CHECK-NEXT: Branch relaxation pass
; CHECK-NEXT: RISCV Remove Back To Back Branches Pass
; CHECK-NEXT: Contiguously Lay Out Funclets
; CHECK-NEXT: Remove Loads Into Fake Uses
; CHECK-NEXT: StackMap Liveness Analysis
Expand Down
3 changes: 2 additions & 1 deletion llvm/test/CodeGen/RISCV/O3-pipeline.ll
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,10 @@
; CHECK-NEXT: Insert XRay ops
; CHECK-NEXT: Implement the 'patchable-function' attribute
; CHECK-NEXT: Machine Copy Propagation Pass
; CHECK-NEXT: Branch relaxation pass
; CHECK-NEXT: RISC-V Make Compressible
; CHECK-NEXT: Unpack machine instruction bundles
; CHECK-NEXT: Branch relaxation pass
; CHECK-NEXT: RISCV Remove Back To Back Branches Pass
; CHECK-NEXT: Contiguously Lay Out Funclets
; CHECK-NEXT: Remove Loads Into Fake Uses
; CHECK-NEXT: StackMap Liveness Analysis
Expand Down