This is an archive of the discontinued LLVM Phabricator instance.

add riscv32e to the llvm
Needs ReviewPublic

Authored by xudaliang.pku on Oct 15 2018, 7:59 AM.

Details

Reviewers
asb
Summary
  1. reserved x16-x31reg
  2. RV32E ABI
    1. calling convention support
    2. stack align
  3. add riscv32e target( --target=riscv32e )

Diff Detail

Event Timeline

xudaliang.pku created this revision.Oct 15 2018, 7:59 AM
xudaliang.pku edited the summary of this revision. (Show Details)Oct 15 2018, 8:15 AM
rogfer01 added inline comments.Oct 15 2018, 8:34 AM
lib/Target/RISCV/RISCVISelLowering.cpp
42

I wonder if we can avoid this global variable and instead use Subtarget.isEmbed() where needed.

This will require passing a const RISCVSubtarget& to some functions (like CC_RISCV) but given that they are local to the file, I think that would be reasonable.

simoncook added inline comments.Oct 15 2018, 8:44 AM
lib/Support/Triple.cpp
46

This should be on the same line as the case statement, similar to the rest of the table.

lib/Target/RISCV/RISCVFrameLowering.cpp
27

I think this would be more readable as /*StackAlignment=*/ STI.isEmbed() ? 4 : 16,

lib/Target/RISCV/RISCVFrameLowering.h
24

Did you mean to put a 'T' on the end of this variable name?

Why do we need another triple? Can we not just use the feature flag in the few cases where the code needs to tell the difference? There's a whole lot of boilerplate here just to add the new triple.

kito-cheng added a comment.EditedOct 15 2018, 7:26 PM

rv32e arch and ilp32e ABI is decoupling in GCC, that's mean rv32i with ilp32e is possible, so I would suggest separate two thing.

rv32e arch and ilp32e ABI is decoupling is GCC, that's mean rv32i with ilp32e is possible, so I would suggest separate two thing.

Very true.

In embedded systems it can often make sense to use the ilp32e ABI even on CPUs with 32 registers because there are significantly fewer argument and temporary registers, making interrupt response time shorter. All the high 16 registers are callee-save, which means you can use them if required but you need to save them first and restore them after.

asb added a comment.Oct 15 2018, 8:11 PM

Hi Daliang and welcome to the LLVM community.

Many thanks for your work on RV32E support. In our discussion on the sw-dev mailing list I suggested splitting this task into the MC layer, calling convention, and codegen support. I'd recommend preparing patches in a way that does this. Each of these elements should come along with tests.

You shouldn't need to have riscv32e as a new triple. Just rely on the SubtargetFeature - this should remove a lot of code from this patch.

rv32e arch and ilp32e ABI is decoupling is GCC, that's mean rv32i with ilp32e is possible, so I would suggest separate two thing.

Very true.

In embedded systems it can often make sense to use the ilp32e ABI even on CPUs with 32 registers because there are significantly fewer argument and temporary registers, making interrupt response time shorter. All the high 16 registers are callee-save, which means you can use them if required but you need to save them first and restore them after.

Thanks for your advice.
I think decouple the rv32e and ilp32e is great. I have not considered it before. I will do it later.
But, I was not sure what rv32e and ilp32e really meaans. There is still something not clearly. Is the march rv32e only means that the regs only contains x0-x15 ? And the ABI ilp32e means Stack Alignment and calling convention, etc (see in https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#rv32e-calling-convention )?

But, I was not sure what rv32e and ilp32e really meaans. There is still something not clearly. Is the march rv32e only means that the regs only contains x0-x15 ? And the ABI ilp32e means Stack Alignment and calling convention, etc (see in https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#rv32e-calling-convention )?

That's correct.

rv32e is just about hardware. Registers x16-x31 do not exist.

ilp32e is just about software. The function call convention and stack layout, as you say.

The link you point to should have that section renamed.

In D53291#1266088, @asb wrote:

Hi Daliang and welcome to the LLVM community.

Many thanks for your work on RV32E support. In our discussion on the sw-dev mailing list I suggested splitting this task into the MC layer, calling convention, and codegen support. I'd recommend preparing patches in a way that does this. Each of these elements should come along with tests.

You shouldn't need to have riscv32e as a new triple. Just rely on the SubtargetFeature - this should remove a lot of code from this patch.

I will spilt the patch later as you said. It is the first time I have upload my patch. So, maybe I consider too little about how to show my work.I will do better next time.
Just like you and kito-cheng @kito-cheng have said. I had not added the triple:riscv32e in my work until I found clang-side needed to use that triple:riscv32e in the discussion on the sw-dev mailing list. The code in clang is like that

class RISCVABIInfo : public DefaultABIInfo {
private:
  unsigned XLen; // Size of the integer ('x') registers in bits.
  static const int NumArgGPRs = 8;

public:
  RISCVABIInfo(CodeGen::CodeGenTypes &CGT, unsigned XLen)
      : DefaultABIInfo(CGT), XLen(XLen) { }
};

Because the RISCVABIInfo can't get the SubtargetFeatures, NumArgGPRs will always be 8 int the RISCVABIInfo even if the march=rv32e. So I have no way except for adding the triple:riscv32e. you can also see what I have changed the clang in https://reviews.llvm.org/D53293 about How I changed the NumArgGPRs by using triple:target. I was very confused how to solve this problem.Maybe a better way later.

lib/Support/Triple.cpp
46

Because of the first time using this tools to upload my patch, I was not familiar with it.
I feel very sorry to have some stupid mistakes in my patch. that you have prososed to me. I will modify them soonly

lib/Target/RISCV/RISCVISelLowering.cpp
42

I was not sure whether passing an argument like const RISCVSubtarget& to the functions (like CC_RISCV) may change too much.
Because it change the interface of the function. I have considered that way before.
I will try it later

That's correct.

rv32e is just about hardware. Registers x16-x31 do not exist.

ilp32e is just about software. The function call convention and stack layout, as you say.

The link you point to should have that section renamed.

OK , Thanks for your verify about the march and the mabi. I will do it soon.

In D53291#1266088, @asb wrote:

Hi Daliang and welcome to the LLVM community.

Many thanks for your work on RV32E support. In our discussion on the sw-dev mailing list I suggested splitting this task into the MC layer, calling convention, and codegen support. I'd recommend preparing patches in a way that does this. Each of these elements should come along with tests.

You shouldn't need to have riscv32e as a new triple. Just rely on the SubtargetFeature - this should remove a lot of code from this patch.

I will spilt the patch later as you said. It is the first time I have upload my patch. So, maybe I consider too little about how to show my work.I will do better next time.
Just like you and kito-cheng @kito-cheng have said. I had not added the triple:riscv32e in my work until I found clang-side needed to use that triple:riscv32e in the discussion on the sw-dev mailing list. The code in clang is like that

class RISCVABIInfo : public DefaultABIInfo {
private:
  unsigned XLen; // Size of the integer ('x') registers in bits.
  static const int NumArgGPRs = 8;

public:
  RISCVABIInfo(CodeGen::CodeGenTypes &CGT, unsigned XLen)
      : DefaultABIInfo(CGT), XLen(XLen) { }
};

Because the RISCVABIInfo can't get the SubtargetFeatures, NumArgGPRs will always be 8 int the RISCVABIInfo even if the march=rv32e. So I have no way except for adding the triple:riscv32e. you can also see what I have changed the clang in https://reviews.llvm.org/D53293 about How I changed the NumArgGPRs by using triple:target. I was very confused how to solve this problem.Maybe a better way later.

The RISCVABIInfo constructor is called by RISCVTargetCodeGenInfo's constructor, which in turn is called by CodeGenModule::getTargetCodeGenInfo. CodeGenModule has a getTarget() which should give you access to the feature flags?

The RISCVABIInfo constructor is called by RISCVTargetCodeGenInfo's constructor, which in turn is called by CodeGenModule::getTargetCodeGenInfo. CodeGenModule has a getTarget() which should give you access to the feature flags?

Thanks for your advice, I will check the CodeGenModule and RISCVTargetCodeGenInfo's code to solve that problem I have proposed. In that way , we can remove the triple:riscv32e. That will be great!

nhaehnle removed a subscriber: nhaehnle.Oct 17 2018, 3:16 AM
lenary added a subscriber: lenary.Jul 31 2019, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 7:33 AM
lenary removed a subscriber: lenary.Jul 31 2019, 7:33 AM
lenary added a subscriber: lenary.Aug 1 2019, 5:04 AM

Are you have plans to promote the codegen of rv32e, if not, I want to try it

Are you have plans to promote the codegen of rv32e, if not, I want to try it

It was my plan to work on it further, but that won't happen. The place to start is with support for the ilp32e ABI, and then add rv32e support after that.

One thing we don't want, as I understand, is a separate triple to denote rv32e - the use of -march and -mabi should be enough to not need a separate triple.

Are you have plans to promote the codegen of rv32e, if not, I want to try it

It was my plan to work on it further, but that won't happen. The place to start is with support for the ilp32e ABI, and then add rv32e support after that.

My patch for ilp32e support is here: D70401 - I've found a little time to go over it this week, and am working on some improvements as we speak. Hopefully it will be ready to review by the end of the week.

One thing we don't want, as I understand, is a separate triple to denote rv32e - the use of -march and -mabi should be enough to not need a separate triple.