Skip to content

Conversation

@icymanred
Copy link

This pr adds support for the multiple single elements variant of the vld1 instruction.
A new struct DoubleWordRegisterList is added to represent register lists and a function ReadRegisterList to read it is added.
A utility function GetDataTypeSize is also added to return the size of a instruction datatype in bits.
A visual comparison of llil before and after.

Before

image # After image

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ icymanred
❌ plafosse
You have signed the CLA already but the status is still pending? Let us recheck it.

@plafosse plafosse requested a review from Copilot October 28, 2025 12:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for the multiple single elements variant of the VLD1 instruction in ARMv7 architecture. The changes include utility functions for handling register lists and data type size calculations, along with the implementation of VLD1 instruction lifting to LLIL.

Key changes:

  • Added GetDataTypeSize function to convert data type enums to bit sizes
  • Added ReadRegisterList function to parse register list operands using platform-specific intrinsics
  • Implemented VLD1 instruction handling with proper register list iteration and memory load operations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
arch/armv7/il.cpp Added utility functions and VLD1 instruction implementation with register list handling and memory operations
arch/armv7/armv7_disasm/armv7.h Added DoubleWordRegisterList struct definition and formatting cleanup for ieee754_double union

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +5130 to +5153
ConditionExecute(addrSize, instr.cond, instr, il, [&](size_t addrSize, Instruction& instr, LowLevelILFunction& il) {
switch (op1.cls)
{
case OperandClass::REG_LIST_DOUBLE:
DoubleWordRegisterList reglist = ReadRegisterList(op1);
uint32_t dregsize = get_register_size(REG_D0);
uint32_t regsize = get_register_size(op2.reg);
uint32_t dataSizeInBytes = GetDataTypeSize(instr.dataType) / 8;
for (unsigned int i = 0; i < reglist.size; i++)
{
uint32_t curOffset = i * dataSizeInBytes;
uint32_t curregind = REG_D0 + reglist.start + i;

il.AddInstruction(il.SetRegister(dregsize, curregind,
il.Load(dataSizeInBytes, il.Add(regsize, ILREG(op2), il.Const(regsize, curOffset)))));
}
if (op2.flags.wb)
{
il.AddInstruction(il.SetRegister(
regsize, op2.reg, il.Add(regsize, ILREG(op2), il.Const(regsize, dataSizeInBytes * reglist.size))));
}
}
});
break;
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The ConditionExecute call has incorrect parameters. Based on the function signature shown in the context, ConditionExecute takes (LowLevelILFunction& il, Condition cond, ExprId trueCase), but this call passes addrSize as the first parameter and uses a lambda that doesn't return an ExprId.

Suggested change
ConditionExecute(addrSize, instr.cond, instr, il, [&](size_t addrSize, Instruction& instr, LowLevelILFunction& il) {
switch (op1.cls)
{
case OperandClass::REG_LIST_DOUBLE:
DoubleWordRegisterList reglist = ReadRegisterList(op1);
uint32_t dregsize = get_register_size(REG_D0);
uint32_t regsize = get_register_size(op2.reg);
uint32_t dataSizeInBytes = GetDataTypeSize(instr.dataType) / 8;
for (unsigned int i = 0; i < reglist.size; i++)
{
uint32_t curOffset = i * dataSizeInBytes;
uint32_t curregind = REG_D0 + reglist.start + i;
il.AddInstruction(il.SetRegister(dregsize, curregind,
il.Load(dataSizeInBytes, il.Add(regsize, ILREG(op2), il.Const(regsize, curOffset)))));
}
if (op2.flags.wb)
{
il.AddInstruction(il.SetRegister(
regsize, op2.reg, il.Add(regsize, ILREG(op2), il.Const(regsize, dataSizeInBytes * reglist.size))));
}
}
});
break;
{
// Build the IL instructions for the VLD1 operation
switch (op1.cls)
{
case OperandClass::REG_LIST_DOUBLE:
{
DoubleWordRegisterList reglist = ReadRegisterList(op1);
uint32_t dregsize = get_register_size(REG_D0);
uint32_t regsize = get_register_size(op2.reg);
uint32_t dataSizeInBytes = GetDataTypeSize(instr.dataType) / 8;
for (unsigned int i = 0; i < reglist.size; i++)
{
uint32_t curOffset = i * dataSizeInBytes;
uint32_t curregind = REG_D0 + reglist.start + i;
il.AddInstruction(il.SetRegister(dregsize, curregind,
il.Load(dataSizeInBytes, il.Add(regsize, ILREG(op2), il.Const(regsize, curOffset)))));
}
if (op2.flags.wb)
{
il.AddInstruction(il.SetRegister(
regsize, op2.reg, il.Add(regsize, ILREG(op2), il.Const(regsize, dataSizeInBytes * reglist.size))));
}
break;
}
default:
break;
}
// Use il.Nop() as the trueCase since the instructions have already been added
ConditionExecute(il, instr.cond, il.Nop());
break;
}

Copilot uses AI. Check for mistakes.
Copy link
Member

@plafosse plafosse Oct 28, 2025

Choose a reason for hiding this comment

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

There are two definitions of ConditionExecute consider this one

void ConditionExecute(size_t addrSize, Condition cond, Instruction& instr, LowLevelILFunction& il,
	std::function<void (size_t addrSize, Instruction& instr, LowLevelILFunction& il)> conditionalCode)
{

@plafosse plafosse requested a review from Copilot October 28, 2025 13:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

case DT_F64:
case DT_I64:
case DT_64:
return 64;
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The switch statement lacks a default case and doesn't return a value for unhandled DataType values. This will cause undefined behavior if an unsupported DataType is passed. Add a default case that returns 0 or asserts/logs an error.

Suggested change
return 64;
return 64;
default:
return 0;

Copilot uses AI. Check for mistakes.
);
break;
case ARMV7_VLD1:
ConditionExecute(addrSize, instr.cond, instr, il, [&](size_t addrSize, Instruction& instr, LowLevelILFunction& il) {
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The ConditionExecute call signature doesn't match the function definition shown in the context. The function expects (LowLevelILFunction& il, Condition cond, ExprId trueCase) but is being called with (addrSize, instr.cond, instr, il, lambda). This indicates either the wrong overload is being used or the function signature is incorrect.

Copilot uses AI. Check for mistakes.
Comment on lines +5127 to +5130
switch (op1.cls)
{
case OperandClass::REG_LIST_DOUBLE:
DoubleWordRegisterList reglist = ReadRegisterList(op1);
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The switch statement has no default case and no break statement after the REG_LIST_DOUBLE case. This will cause fall-through behavior and potential undefined behavior if other operand classes are encountered. Add a break statement after line 5146 and consider adding a default case.

Copilot uses AI. Check for mistakes.
{
il.AddInstruction(il.SetRegister(
regsize, op2.reg, il.Add(regsize, ILREG(op2), il.Const(regsize, dataSizeInBytes * reglist.size))));
}
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The switch statement has no default case and no break statement after the REG_LIST_DOUBLE case. This will cause fall-through behavior and potential undefined behavior if other operand classes are encountered. Add a break statement after line 5146 and consider adding a default case.

Suggested change
}
}
break;
default:
il.AddInstruction(il.Unimplemented());
break;

Copilot uses AI. Check for mistakes.
@Vector35 Vector35 deleted a comment from Copilot AI Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants