Skip to content
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.

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

[COREV] Define ALU instructions and add tests #10

Merged
merged 1 commit into from Mar 4, 2021

Conversation

serkm
Copy link
Contributor

@serkm serkm commented Feb 23, 2021

No description provided.

@flip1995
Copy link
Member

Thanks for the PRs! I probably have time to review those next week. In the meantime, can you resolve the merge conflicts please?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

IIUC the bitmanip extension is not implemented by the CORE-V GCC compiler, since it is superseded by the RISC-V B extension (once this becomes ratified). I'm not sure if we should include those here. cc @jeremybennett

Tests LGTM

@@ -176,6 +177,178 @@ let Predicates = [HasExtXCoreVMac], hasSideEffects = 0, mayLoad = 0, mayStore =
Sched<[]>;
} // Predicates = [HasExtXCoreVMac], hasSideEffects = 0, mayLoad = 0, mayStore = 0

let Predicates = [HasExtXCoreVAlu], hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

class RVInstAlu_rii<bits<2> funct2, bits<3> funct3, dag outs, dag ins,
string opcodestr, string argstr, list<dag> pattern>
: RVInst<outs, ins, opcodestr, argstr, pattern, InstFormatOther> {
bits<5> Is3;
Copy link
Member

Choose a reason for hiding this comment

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

Immediate are usually called imm<#bits>, so this would be called

Suggested change
bits<5> Is3;
bits<5> imm5;

Since there is no reference on how to name immediate, if there are two with the same size, I suggest to use imm5_1 imm5_2 here.

let Predicates = [HasExtXCoreVAlu], hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {

// Bit manipulation operations
def CV_EXTRACT : RVInstAlu_rii<0b11, 0b000, (outs GPR:$rd), (ins GPR:$rs1, uimm5:$Is3, uimm5:$Is2),
Copy link
Member

Choose a reason for hiding this comment

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

Also here (and everywhere below), the immediate naming convention should be applied

Suggested change
def CV_EXTRACT : RVInstAlu_rii<0b11, 0b000, (outs GPR:$rd), (ins GPR:$rs1, uimm5:$Is3, uimm5:$Is2),
def CV_EXTRACT : RVInstAlu_rii<0b11, 0b000, (outs GPR:$rd), (ins GPR:$rs1, uimm5:$imm5, uimm5:$imm5),

Comment on lines 216 to 218
def CV_BITREV : RVInstAlu_rii<0b11, 0b101, (outs GPR:$rd), (ins GPR:$rs1, uimm5:$Is3, uimm5:$Is2),
"cv.bitrev", "$rd, $rs1, $Is3, $Is2", []>,
Sched<[]>;
Copy link
Member

Choose a reason for hiding this comment

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

This instruction only takes a uimm2 as the third argument. But then again, the first 3 bits just get ignored, if a uimm5 is passed.

Maybe create a special operand type for this? I'm not sure if that is necessary though.

"cv.bitrev", "$rd, $rs1, $Is3, $Is2", []>,
Sched<[]>;

let DecoderNamespace = "COREV_ALU" in
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary for the cv.ror instruction?

Comment on lines 370 to 380
if (!STI.getFeatureBits()[RISCV::FeatureExtXCoreVAlu]) {
LLVM_DEBUG(
dbgs() << "Trying COREV_ALU32 table (32-bit Instruction):\n");
// Calling the auto-generated decoder function.
Result = decodeInstruction(DecoderTableCOREV_ALU32, MI, Insn, Address,
this, STI);
if (Result != MCDisassembler::Fail) {
Size = 4;
return Result;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this only deals with the cv.ror command? I don't quite get, why this is necessary. AFAICT no CORE-V instructions can even reach this code, since all of the instructions have an opcode ending in 0b11, since cv32e40p is a 32-bit platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I must have made a mistake here.

The different decoding namespace is necessary, because otherwise the disassembler creates an <unknown> when there is an ambiguous instruction encoding, making some tests on the disassembler fail.

Anyway, this shouldn't matter, since cv.ror isn't supported anymore.

@jeremybennett
Copy link
Collaborator

IIUC the bitmanip extension is not implemented by the CORE-V GCC compiler, since it is superseded by the RISC-V B extension (once this becomes ratified). I'm not sure if we should include those here. cc @jeremybennett

Tests LGTM

@Serka

IIUC the bitmanip extension is not implemented by the CORE-V GCC compiler, since it is superseded by the RISC-V B extension (once this becomes ratified). I'm not sure if we should include those here. cc @jeremybennett

Tests LGTM

@SerkanMuhcu @flip1995 You are correct that we are not implementing the PULP bit manip extension in the CORE-V GCC. It is not implemented n the standard CV32E40P and will be replaced by the standard B extension in the V2 chip.

@serkm
Copy link
Contributor Author

serkm commented Mar 1, 2021

Ok, I'll remove the bitmanip instructions.

@jeremybennett Did I understand correctly that the SIMD instructions are not supported aswell?

@flip1995
Copy link
Member

flip1995 commented Mar 2, 2021

Yes, the SIMD instructions are superseded by the RISC-V P extension.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Last 2 NITs, then this should be good to go 👍

Comment on lines 182 to 192
bits<12> Imm12;
bits<5> Imm5;
bits<5> rs1;

let Inst{31} = Imm12{11};
let Inst{30-25} = Imm12{9-4};
let Inst{24-20} = Imm5;
let Inst{19-15} = rs1;
let Inst{14-12} = funct3;
let Inst{11-8} = Imm12{3-0};
let Inst{7} = Imm12{10};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bits<12> Imm12;
bits<5> Imm5;
bits<5> rs1;
let Inst{31} = Imm12{11};
let Inst{30-25} = Imm12{9-4};
let Inst{24-20} = Imm5;
let Inst{19-15} = rs1;
let Inst{14-12} = funct3;
let Inst{11-8} = Imm12{3-0};
let Inst{7} = Imm12{10};
bits<12> imm12;
bits<5> imm5;
bits<5> rs1;
let Inst{31} = imm12{11};
let Inst{30-25} = imm12{9-4};
let Inst{24-20} = imm5;
let Inst{19-15} = rs1;
let Inst{14-12} = funct3;
let Inst{11-8} = imm12{3-0};
let Inst{7} = imm12{10};

Comment on lines 285 to 290
def CV_BEQIMM : RVInstImmBranch<0b010, (outs), (ins GPR:$rs1, simm5:$Imm5, simm13_lsb0:$Imm12),
"cv.beqimm", "$rs1, $Imm5, $Imm12", []>,
Sched<[]>;
def CV_BNEIMM : RVInstImmBranch<0b011, (outs), (ins GPR:$rs1, simm5:$Imm5, simm13_lsb0:$Imm12),
"cv.bneimm", "$rs1, $Imm5, $Imm12", []>,
Sched<[]>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def CV_BEQIMM : RVInstImmBranch<0b010, (outs), (ins GPR:$rs1, simm5:$Imm5, simm13_lsb0:$Imm12),
"cv.beqimm", "$rs1, $Imm5, $Imm12", []>,
Sched<[]>;
def CV_BNEIMM : RVInstImmBranch<0b011, (outs), (ins GPR:$rs1, simm5:$Imm5, simm13_lsb0:$Imm12),
"cv.bneimm", "$rs1, $Imm5, $Imm12", []>,
Sched<[]>;
def CV_BEQIMM : RVInstImmBranch<0b010, (outs), (ins GPR:$rs1, simm5:$imm5, simm13_lsb0:$imm12),
"cv.beqimm", "$rs1, $imm5, $imm12", []>,
Sched<[]>;
def CV_BNEIMM : RVInstImmBranch<0b011, (outs), (ins GPR:$rs1, simm5:$imm5, simm13_lsb0:$imm12),
"cv.bneimm", "$rs1, $imm5, $imm12", []>,
Sched<[]>;

(CV_MULSRN GPR:$rs1, GPR:$rs2, uimm5:$Is3)>;
def : Pat<(shiftRound (mulhhs GPR:$rs1, GPR:$rs2), uimm5:$Is3),
(CV_MULHHSRN GPR:$rs1, GPR:$rs2, uimm5:$Is3)>;
def : Pat<(sra (muls GPR:$rs1, GPR:$rs2), uimm5:$imm5),
Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks, I missed this in my last review.

@flip1995
Copy link
Member

flip1995 commented Mar 2, 2021

Thanks! Please squash your commits and this should be ready to merge.

Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
@flip1995
Copy link
Member

flip1995 commented Mar 2, 2021

@jeremybennett This is ready to get merged 🚀

@jeremybennett
Copy link
Collaborator

Thanks for all the work on this. Good to merge.

@jeremybennett jeremybennett merged commit 51283e0 into openhwgroup:development Mar 4, 2021
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Aug 18, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Aug 18, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Aug 18, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Aug 19, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Aug 19, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 25, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 26, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Oct 26, 2022
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
ChunyuLiao pushed a commit to ChunyuLiao/corev-llvm-project that referenced this pull request Nov 16, 2022
Found by msan -fsanitize-memory-use-after-dtor.

==8259==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55dbec54d2b8 in dtorRecord(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:150:22
    #1 0x55dbec54bfcf in dtorArrayDesc(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:97:7
    openhwgroup#2 0x55dbec508578 in invokeDtor clang/lib/AST/Interp/InterpBlock.h:79:7
    openhwgroup#3 0x55dbec508578 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:55:19
    openhwgroup#4 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    openhwgroup#5 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    openhwgroup#6 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
    openhwgroup#7 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    openhwgroup#8 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    openhwgroup#9 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
    openhwgroup#10 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
    openhwgroup#11 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
    openhwgroup#12 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
    openhwgroup#13 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
    openhwgroup#14 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
    openhwgroup#15 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
    openhwgroup#16 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
    openhwgroup#17 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    openhwgroup#18 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
    openhwgroup#19 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
    openhwgroup#20 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
    openhwgroup#21 0x7f9be07fa632 in __libc_start_main
    openhwgroup#22 0x55dbe6a702e9 in _start

  Member fields were destroyed
    #0 0x55dbe6a7da5d in __sanitizer_dtor_callback_fields compiler-rt/lib/msan/msan_interceptors.cpp:949:5
    #1 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:479:7
    openhwgroup#2 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:612:3
    openhwgroup#3 0x55dbec5094ac in llvm::SmallVector<clang::interp::Record::Base, 8u>::~SmallVector() llvm/include/llvm/ADT/SmallVector.h:1207:3
    openhwgroup#4 0x55dbec508e79 in clang::interp::Record::~Record() clang/lib/AST/Interp/Record.h:24:7
    openhwgroup#5 0x55dbec508612 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:49:26
    openhwgroup#6 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    openhwgroup#7 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    openhwgroup#8 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
    openhwgroup#9 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    openhwgroup#10 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    openhwgroup#11 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
    openhwgroup#12 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
    openhwgroup#13 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
    openhwgroup#14 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
    openhwgroup#15 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
    openhwgroup#16 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
    openhwgroup#17 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
    openhwgroup#18 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
    openhwgroup#19 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    openhwgroup#20 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
    openhwgroup#21 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
    openhwgroup#22 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
    openhwgroup#23 0x7f9be07fa632 in __libc_start_main
    openhwgroup#24 0x55dbe6a702e9 in _start
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Feb 13, 2023
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
CharKeaney pushed a commit to CharKeaney/corev-llvm-project that referenced this pull request Feb 14, 2023
Signed-off-by: Serkan Muhcu <serkan.muhcu@student.uni-tuebingen.de>
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.

None yet

3 participants