Skip to content

[TII] Split isTrivialReMaterializable into two versions [nfc] #160377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 25, 2025
Merged

[TII] Split isTrivialReMaterializable into two versions [nfc] #160377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c90a833
[TII] Split isTrivialReMaterializable into two versions [nfc]
preames Sep 23, 2025
391c5a7
Update llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
preames Sep 24, 2025
53c5426
Update comments to match code behavior
preames Sep 24, 2025
7d6b8a4
Merge branch 'main' into pr-tti-remat-hook-split-nfc
preames Sep 24, 2025
fce5aaf
Clang-format
preames Sep 24, 2025
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
18 changes: 15 additions & 3 deletions llvm/include/llvm/CodeGen/TargetInstrInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
/// registers so that the instructions result is independent of the place
/// in the function.
bool isTriviallyReMaterializable(const MachineInstr &MI) const {
if (!isReMaterializable(MI))
return false;
for (const MachineOperand &MO : MI.all_uses()) {
if (MO.getReg().isVirtual())
return false;
}
return true;
}

/// Return true if the instruction would be materializable at a point
/// in the containing function where all virtual register uses were
/// known to be live and available in registers.
bool isReMaterializable(const MachineInstr &MI) const {
return (MI.getOpcode() == TargetOpcode::IMPLICIT_DEF &&
MI.getNumOperands() == 1) ||
(MI.getDesc().isRematerializable() &&
Expand All @@ -194,10 +207,9 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
protected:
/// For instructions with opcodes for which the M_REMATERIALIZABLE flag is
/// set, this hook lets the target specify whether the instruction is actually
/// trivially rematerializable, taking into consideration its operands. This
/// rematerializable, taking into consideration its operands. This
/// predicate must return false if the instruction has any side effects other
/// than producing a value, or if it requres any address registers that are
/// not always available.
/// than producing a value.
virtual bool isReMaterializableImpl(const MachineInstr &MI) const;

/// This method commutes the operands of the given machine instruction MI.
Expand Down
6 changes: 1 addition & 5 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2212,11 +2212,7 @@ findPrologueEndLoc(const MachineFunction *MF) {
-> std::optional<std::pair<const MachineInstr *, bool>> {
// Is this instruction trivial data shuffling or frame-setup?
bool isCopy = (TII.isCopyInstr(MI) ? true : false);
bool isTrivRemat =
TII.isTriviallyReMaterializable(MI) &&
llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
return MO.getReg().isVirtual();
});
bool isTrivRemat = TII.isTriviallyReMaterializable(MI);
Copy link
Collaborator

@mikaelholmen mikaelholmen Sep 25, 2025

Choose a reason for hiding this comment

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

I'm a little puzzled.
This patch changes the behavior of the code here.

So before, for isTrivRemat to be set, all registers in the MI had to be virtual.
With the patch we call

  /// Return true if the instruction is trivially rematerializable, meaning it
  /// has no side effects and requires no operands that aren't always available.
  /// This means the only allowed uses are constants and unallocatable physical
  /// registers so that the instructions result is independent of the place
  /// in the function.
  bool isTriviallyReMaterializable(const MachineInstr &MI) const {
    if (!isReMaterializable(MI))
      return false;
    for (const MachineOperand &MO : MI.all_uses()) {
      if (MO.getReg().isVirtual())
        return false;
    }
    return true;
  }

which will return false if any of the registers in the MI is virtual.

Switching
if (MO.getReg().isVirtual())
to
if (!MO.getReg().isVirtual())
makes it behave as before but then I guess it doesn't do what the method comment says.

I noticed this for a downstream testcase where the prologue_end placement changed with ca2e8fc and then again with this patch.

Copy link
Collaborator Author

@preames preames Sep 25, 2025

Choose a reason for hiding this comment

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

Ah, you are completely correct. Oops. This was a major bug in #160319. The check should have been "are all of the operands not a virtual register", but this is not what actually got written and landed.

I think this change accidentally fixed this (by doing what the intention had been all along), but boy does that make both patch descriptions misleading. Do you think this is worth a revert-and-reapply on both?

Copy link
Collaborator

@mikaelholmen mikaelholmen Sep 25, 2025

Choose a reason for hiding this comment

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

Ok, so now it's doing what is intended. Good!

Ah, I don't know if you need to fiddle with revert/reapply, I just got confused about the ping/pong behavior of our testcase and that's clear now.
Thanks.

Copy link
Collaborator Author

@preames preames Sep 25, 2025

Choose a reason for hiding this comment

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

Thank you for flagging this. I'm going to put a note on each review for later reference as this could be quite confusing later when digging through history.

bool isFrameSetup = MI.getFlag(MachineInstr::FrameSetup);

if (!isFrameSetup && MI.getDebugLoc()) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/CalcSpillWeights.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ bool VirtRegAuxInfo::isRematerializable(const LiveInterval &LI,
assert(MI && "Dead valno in interval");
}

if (!TII.isTriviallyReMaterializable(*MI))
if (!TII.isReMaterializable(*MI))
return false;
}
return true;
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/CodeGen/LiveRangeEdit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ void LiveRangeEdit::scanRemattable() {
MachineInstr *DefMI = LIS.getInstructionFromIndex(OrigVNI->def);
if (!DefMI)
continue;
if (TII.isTriviallyReMaterializable(*DefMI))
if (TII.isReMaterializable(*DefMI))
Remattable.insert(OrigVNI);
}
ScannedRemattable = true;
Expand Down Expand Up @@ -387,7 +387,7 @@ void LiveRangeEdit::eliminateDeadDef(MachineInstr *MI, ToShrinkSet &ToShrink) {
// register uses. That may provoke RA to split an interval at the KILL
// and later result in an invalid live segment end.
if (isOrigDef && DeadRemats && !HasLiveVRegUses &&
TII.isTriviallyReMaterializable(*MI)) {
TII.isReMaterializable(*MI)) {
LiveInterval &NewLI = createEmptyIntervalFrom(Dest, false);
VNInfo::Allocator &Alloc = LIS.getVNInfoAllocator();
VNInfo *VNI = NewLI.getNextValue(Idx, Alloc);
Expand Down
27 changes: 4 additions & 23 deletions llvm/lib/CodeGen/MachineLICM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,6 @@ namespace {

bool IsGuaranteedToExecute(MachineBasicBlock *BB, MachineLoop *CurLoop);

bool isTriviallyReMaterializable(const MachineInstr &MI) const;

void EnterScope(MachineBasicBlock *MBB);

void ExitScope(MachineBasicBlock *MBB);
Expand Down Expand Up @@ -771,23 +769,6 @@ bool MachineLICMImpl::IsGuaranteedToExecute(MachineBasicBlock *BB,
return true;
}

/// Check if \p MI is trivially remateralizable and if it does not have any
/// virtual register uses. Even though rematerializable RA might not actually
/// rematerialize it in this scenario. In that case we do not want to hoist such
/// instruction out of the loop in a belief RA will sink it back if needed.
bool MachineLICMImpl::isTriviallyReMaterializable(
const MachineInstr &MI) const {
if (!TII->isTriviallyReMaterializable(MI))
return false;

for (const MachineOperand &MO : MI.all_uses()) {
if (MO.getReg().isVirtual())
return false;
}

return true;
}

void MachineLICMImpl::EnterScope(MachineBasicBlock *MBB) {
LLVM_DEBUG(dbgs() << "Entering " << printMBBReference(*MBB) << '\n');

Expand Down Expand Up @@ -1300,9 +1281,9 @@ bool MachineLICMImpl::IsProfitableToHoist(MachineInstr &MI,
return false;
}

// Rematerializable instructions should always be hoisted providing the
// register allocator can just pull them down again when needed.
if (isTriviallyReMaterializable(MI))
// Trivially rematerializable instructions should always be hoisted
// providing the register allocator can just pull them down again when needed.
if (TII->isTriviallyReMaterializable(MI))
return true;

// FIXME: If there are long latency loop-invariant instructions inside the
Expand Down Expand Up @@ -1386,7 +1367,7 @@ bool MachineLICMImpl::IsProfitableToHoist(MachineInstr &MI,

// High register pressure situation, only hoist if the instruction is going
// to be remat'ed.
if (!isTriviallyReMaterializable(MI) &&
if (!TII->isTriviallyReMaterializable(MI) &&
!MI.isDereferenceableInvariantLoad()) {
LLVM_DEBUG(dbgs() << "Can't remat / high reg-pressure: " << MI);
return false;
Expand Down
5 changes: 1 addition & 4 deletions llvm/lib/CodeGen/RegAllocScore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,7 @@ llvm::calculateRegAllocScore(const MachineFunction &MF,
},
[&](const MachineInstr &MI) {
auto *TTI = MF.getSubtarget().getInstrInfo();
return TTI->isTriviallyReMaterializable(MI) &&
llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
return MO.getReg().isVirtual();
});
return TTI->isTriviallyReMaterializable(MI);
});
}

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/RegisterCoalescer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
if (!TII->isAsCheapAsAMove(*DefMI))
return false;

if (!TII->isTriviallyReMaterializable(*DefMI))
if (!TII->isReMaterializable(*DefMI))
return false;

if (!definesFullReg(*DefMI, SrcReg))
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/TargetRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ bool TargetRegisterInfo::shouldRegionSplitForVirtReg(
const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
const MachineRegisterInfo &MRI = MF.getRegInfo();
MachineInstr *MI = MRI.getUniqueVRegDef(VirtReg.reg());
if (MI && TII->isTriviallyReMaterializable(*MI) &&
if (MI && TII->isReMaterializable(*MI) &&
VirtReg.size() > HugeSizeForSplit)
return false;
return true;
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1779,7 +1779,7 @@ bool PreRARematStage::canIncreaseOccupancyOrReduceSpill() {
for (auto MI = Region.first; MI != Region.second; ++MI) {
// The instruction must be trivially rematerializable.
MachineInstr &DefMI = *MI;
if (!isTriviallyReMaterializable(DefMI))
if (!isReMaterializable(DefMI))
Copy link
Collaborator Author

@preames preames Sep 24, 2025

Choose a reason for hiding this comment

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

@arsenm I want to draw your attention to this change. I'm suspecting the use of non-trivial remat might be a latent bug here from the comments, and code structure. When I tried to make this actually trivial, I do see test failures though.

continue;

// We only support rematerializing virtual registers with one definition.
Expand Down Expand Up @@ -2002,8 +2002,8 @@ void PreRARematStage::rematerialize() {
}

// Copied from MachineLICM
bool PreRARematStage::isTriviallyReMaterializable(const MachineInstr &MI) {
if (!DAG.TII->isTriviallyReMaterializable(MI))
bool PreRARematStage::isReMaterializable(const MachineInstr &MI) {
if (!DAG.TII->isReMaterializable(MI))
return false;

for (const MachineOperand &MO : MI.all_uses()) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/GCNSchedStrategy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ class PreRARematStage : public GCNSchedStage {

/// Whether the MI is trivially rematerializable and does not have any virtual
/// register use.
bool isTriviallyReMaterializable(const MachineInstr &MI);
bool isReMaterializable(const MachineInstr &MI);

/// Rematerializes all instructions in PreRARematStage::Rematerializations
/// and stores the achieved occupancy after remat in
Expand Down
5 changes: 1 addition & 4 deletions llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,7 @@ static void query(const MachineInstr &MI, bool &Read, bool &Write,
// Test whether Def is safe and profitable to rematerialize.
static bool shouldRematerialize(const MachineInstr &Def,
const WebAssemblyInstrInfo *TII) {
return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def) &&
llvm::all_of(Def.all_uses(), [](const MachineOperand &MO) {
return MO.getReg().isVirtual();
});
return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def);
}

// Identify the definition for this register at this point. This is a
Expand Down
Loading