[PATCH] D53291: add riscv32e to the llvm

[Daliang Xu via Phabricator via llvm-commits]

xudaliang.pku added a comment.

In https://reviews.llvm.org/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 <https://groups.google.com/a/groups.riscv.org/d/msg/sw-dev/EEd3xSUXcfg/DgeswrTMFQAJ> 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.



================
Comment at: lib/Support/Triple.cpp:46
+  case riscv32e:
+    return "riscv32e";
   case riscv64:        return "riscv64";
----------------
simoncook wrote:
> This should be on the same line as the case statement, similar to the rest of the table.
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


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:42
 
+static bool isRVE = false;
+
----------------
rogfer01 wrote:
> 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.
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


Repository:
  rL LLVM

https://reviews.llvm.org/D53291