Skip to content

[llvm] use 64-bit types for result of getDwarfRegNum (NFC) #109494

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 1 commit into from
Sep 26, 2024
Merged

[llvm] use 64-bit types for result of getDwarfRegNum (NFC) #109494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions llvm/include/llvm/MC/MCRegisterInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,15 +418,15 @@ class MCRegisterInfo {
/// number. Returns -1 if there is no equivalent value. The second
/// parameter allows targets to use different numberings for EH info and
/// debugging info.
int getDwarfRegNum(MCRegister RegNum, bool isEH) const;
virtual int64_t getDwarfRegNum(MCRegister RegNum, bool isEH) const;

/// Map a dwarf register back to a target register. Returns std::nullopt if
/// there is no mapping.
std::optional<MCRegister> getLLVMRegNum(unsigned RegNum, bool isEH) const;
std::optional<MCRegister> getLLVMRegNum(uint64_t RegNum, bool isEH) const;

/// Map a target EH register number to an equivalent DWARF register
/// number.
int getDwarfRegNumFromDwarfEHRegNum(unsigned RegNum) const;
int64_t getDwarfRegNumFromDwarfEHRegNum(uint64_t RegNum) const;

/// Map a target register to an equivalent SEH register
/// number. Returns LLVM register number if there is no equivalent value.
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,15 +570,15 @@ void DwarfDebug::constructAbstractSubprogramScopeDIE(DwarfCompileUnit &SrcCU,
/// debug expression to a register in the forwarded register worklist.
struct FwdRegParamInfo {
/// The described parameter register.
unsigned ParamReg;
uint64_t ParamReg;

/// Debug expression that has been built up when walking through the
/// instruction chain that produces the parameter's value.
const DIExpression *Expr;
};

/// Register worklist for finding call site values.
using FwdRegWorklist = MapVector<unsigned, SmallVector<FwdRegParamInfo, 2>>;
using FwdRegWorklist = MapVector<uint64_t, SmallVector<FwdRegParamInfo, 2>>;
/// Container for the set of registers known to be clobbered on the path to a
/// call site.
using ClobberedRegSet = SmallSet<Register, 16>;
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void DwarfExpression::emitConstu(uint64_t Value) {
}
}

void DwarfExpression::addReg(int DwarfReg, const char *Comment) {
void DwarfExpression::addReg(int64_t DwarfReg, const char *Comment) {
assert(DwarfReg >= 0 && "invalid negative dwarf register number");
assert((isUnknownLocation() || isRegisterLocation()) &&
"location description already locked down");
Expand All @@ -53,7 +53,7 @@ void DwarfExpression::addReg(int DwarfReg, const char *Comment) {
}
}

void DwarfExpression::addBReg(int DwarfReg, int Offset) {
void DwarfExpression::addBReg(int64_t DwarfReg, int64_t Offset) {
assert(DwarfReg >= 0 && "invalid negative dwarf register number");
assert(!isRegisterLocation() && "location description already locked down");
if (DwarfReg < 32) {
Expand All @@ -65,7 +65,7 @@ void DwarfExpression::addBReg(int DwarfReg, int Offset) {
emitSigned(Offset);
}

void DwarfExpression::addFBReg(int Offset) {
void DwarfExpression::addFBReg(int64_t Offset) {
emitOp(dwarf::DW_OP_fbreg);
emitSigned(Offset);
}
Expand Down Expand Up @@ -108,7 +108,7 @@ bool DwarfExpression::addMachineReg(const TargetRegisterInfo &TRI,
return false;
}

int Reg = TRI.getDwarfRegNum(MachineReg, false);
int64_t Reg = TRI.getDwarfRegNum(MachineReg, false);

// If this is a valid register number, emit it.
if (Reg >= 0) {
Expand Down
12 changes: 6 additions & 6 deletions llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,17 @@ class DwarfExpression {
protected:
/// Holds information about all subregisters comprising a register location.
struct Register {
int DwarfRegNo;
int64_t DwarfRegNo;
unsigned SubRegSize;
const char *Comment;

/// Create a full register, no extra DW_OP_piece operators necessary.
static Register createRegister(int RegNo, const char *Comment) {
static Register createRegister(int64_t RegNo, const char *Comment) {
return {RegNo, 0, Comment};
}

/// Create a subregister that needs a DW_OP_piece operator with SizeInBits.
static Register createSubRegister(int RegNo, unsigned SizeInBits,
static Register createSubRegister(int64_t RegNo, unsigned SizeInBits,
const char *Comment) {
return {RegNo, SizeInBits, Comment};
}
Expand Down Expand Up @@ -161,13 +161,13 @@ class DwarfExpression {

/// Emit a DW_OP_reg operation. Note that this is only legal inside a DWARF
/// register location description.
void addReg(int DwarfReg, const char *Comment = nullptr);
void addReg(int64_t DwarfReg, const char *Comment = nullptr);

/// Emit a DW_OP_breg operation.
void addBReg(int DwarfReg, int Offset);
void addBReg(int64_t DwarfReg, int64_t Offset);

/// Emit DW_OP_fbreg <Offset>.
void addFBReg(int Offset);
void addFBReg(int64_t Offset);

/// Emit a partial DWARF register operation.
///
Expand Down
14 changes: 9 additions & 5 deletions llvm/lib/MC/MCRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ unsigned MCRegisterInfo::getSubRegIndex(MCRegister Reg,
return 0;
}

int MCRegisterInfo::getDwarfRegNum(MCRegister RegNum, bool isEH) const {
int64_t MCRegisterInfo::getDwarfRegNum(MCRegister RegNum, bool isEH) const {
const DwarfLLVMRegPair *M = isEH ? EHL2DwarfRegs : L2DwarfRegs;
unsigned Size = isEH ? EHL2DwarfRegsSize : L2DwarfRegsSize;

Expand All @@ -151,24 +151,28 @@ int MCRegisterInfo::getDwarfRegNum(MCRegister RegNum, bool isEH) const {
const DwarfLLVMRegPair *I = std::lower_bound(M, M+Size, Key);
if (I == M+Size || I->FromReg != RegNum)
return -1;
return I->ToReg;
// Consumers need to be able to detect -1 and -2, but at various points
// the numbers move between unsigned and signed representations, as well as
// between 32- and 64-bit representations. We need to convert first to int
// before int64_t for proper sign handling.
return int64_t(int(I->ToReg));
Copy link
Member

Choose a reason for hiding this comment

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

what about doing the following:

auto reg = I->ToReg;
switch (reg) {
  case -1: return -1;
  case -2: return -2;
  default: return reg;
}

Copy link
Contributor Author

@willghatch willghatch Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That might be good. I'm not certain there are not other negative values currently being used as signal values, but if that switch passes the tests then it's probably fine. I'm not certain that's necessarily better, though. But I'm fine with either implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Then, what about int64_t(int32_t(I->ToReg))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the existing code dealing with these register numbers uses the types int and unsigned. The int64_t(int(I->ToReg)) conversion seems more consistent with other usage than int64_t(int32_t(I->ToReg)).

}

std::optional<MCRegister> MCRegisterInfo::getLLVMRegNum(unsigned RegNum,
std::optional<MCRegister> MCRegisterInfo::getLLVMRegNum(uint64_t RegNum,
bool isEH) const {
const DwarfLLVMRegPair *M = isEH ? EHDwarf2LRegs : Dwarf2LRegs;
unsigned Size = isEH ? EHDwarf2LRegsSize : Dwarf2LRegsSize;

if (!M)
return std::nullopt;
DwarfLLVMRegPair Key = { RegNum, 0 };
DwarfLLVMRegPair Key = {unsigned(RegNum), 0};
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be better to have DwarfLLVMRegPair support int64_t? Or is there something that makes you sure that the current conversion is safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tried to do a broader switch to 64-bit types for registers, but it caused many problems. There are various places in code generation where registers are not just treated as 32-bit numbers, but also treat certain bit offsets as flags. So I limited the change as much as possible to just the output of getDwarfRegNum. Keeping the types used by DwarfLLVMRegPair as unsigned preserves the current behaviors. The only way to give a 64-bit output from getDwarfRegNum that actually needs more than 32-bits is to override getDwarfRegNum and provide an implementation that sidesteps these maps. These maps are generated from tablegen register definitions, and don't make sense for an implementation that needs to use a weird encoding that requires 64 bits anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Oof. Got it. Please mention this in the commit description for posterity.

const DwarfLLVMRegPair *I = std::lower_bound(M, M+Size, Key);
if (I != M + Size && I->FromReg == RegNum)
return MCRegister::from(I->ToReg);
return std::nullopt;
}

int MCRegisterInfo::getDwarfRegNumFromDwarfEHRegNum(unsigned RegNum) const {
int64_t MCRegisterInfo::getDwarfRegNumFromDwarfEHRegNum(uint64_t RegNum) const {
// On ELF platforms, DWARF EH register numbers are the same as DWARF
// other register numbers. On Darwin x86, they differ and so need to be
// mapped. The .cfi_* directives accept integer literals as well as
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/Target/Lanai/LanaiRegisterInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ struct LanaiRegisterInfo : public LanaiGenRegisterInfo {
Register getFrameRegister(const MachineFunction &MF) const override;
Register getBaseRegister() const;
bool hasBasePointer(const MachineFunction &MF) const;

int getDwarfRegNum(unsigned RegNum, bool IsEH) const;
Comment on lines -46 to -47
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It caused errors when I switched getDwarfRegNum to be virtual, and there didn't seem to be a reason for it to be there. No tests failed from removing it. I believe it is vestigial, but I'm open to someone telling or showing me why it should be there.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. What you say makes sense.

};

} // end namespace llvm
Expand Down
Loading