[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via cfe-commits]

lenary marked an inline comment as done.
lenary added a comment.

In D70401#3188138 , @zixuan-wu wrote:

> In D70401#3175266 , @khchen wrote:
>
>>> Is it (D70401 ) good enough to solve or 
>>> complete rv32e issue?
>>
>> It need to
>>
>> 1. disallow ilp32e ABI with D ISA extension. 
>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
>> 2. emit predefined marco in header (`__riscv_e`)
>> 3. markSuperRegs for X16-X31
>> 4. update tests after rebase on main.
>
> Nice. If no body objects, @pcwang-thead will take this task and re-draft a 
> review.

Please do feel free to commandeer the current patch. I cannot continue to work 
on it (so, you won't see review comments from me).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a subscriber: pcwang-thead.
zixuan-wu added a comment.

In D70401#3175266 , @khchen wrote:

>> Is it (D70401 ) good enough to solve or 
>> complete rv32e issue?
>
> It need to
>
> 1. disallow ilp32e ABI with D ISA extension. 
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
> 2. emit predefined marco in header (`__riscv_e`)
> 3. markSuperRegs for X16-X31
> 4. update tests after rebase on main.

Nice. If no body objects, @pcwang-thead will take this task and re-draft a 
review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zakk Chen via Phabricator via cfe-commits]

khchen added a comment.

> Is it (D70401 ) good enough to solve or 
> complete rv32e issue?

It need to

1. disallow ilp32e ABI with D ISA extension. 
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/3f81fae0412bb9ad4002a4ade508be7aa5e1599b/riscv-cc.adoc#ilp32e-calling-convention
2. emit predefined marco in header (`__riscv_e`)
3. markSuperRegs for X16-X31
4. update tests after rebase on mani.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.

In D70401#3172750 , @khchen wrote:

> In D70401#3172457 , @zixuan-wu wrote:
>
>> Hi, all. Why is it not continued?
>
> Sorry, I have to work on other tasks so stop the rv32e implementation work.
> Are you interest to finish it? I could share my patches to you.

Is it (D70401 ) good enough to solve or 
complete rv32e issue?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zakk Chen via Phabricator via cfe-commits]

khchen added a comment.

In D70401#3172457 , @zixuan-wu wrote:

> Hi, all. Why is it not continued?

Sorry, I have to work on other tasks so stop the rv32e implementation work.
Are you interest to finish it? I could share my patches to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.
Herald added subscribers: VincentWu, luke957, achieveartificialintelligence.

Hi, all. Why is it not continued?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D70401#2733003 , @khchen wrote:

> Hi, I would like to add ilp32e ABI support in llvm
> Is there anyone working on this?
> It seem the one thing missed is ilp32e ABI should disallow D ISA extension.
> Is there anything else?

Nobody is currently working on this on the lowRISC side, so please do pick this 
up if you're interested. I can't quite recall the open issues I'm afraid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Zakk Chen via Phabricator via cfe-commits]

khchen added a comment.
Herald added a subscriber: vkmr.

Hi, I would like to add ilp32e ABI support in llvm
Is there anyone working on this?
It seem the one thing missed is ilp32e ABI should disallow D ISA extension.
Is there anything else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via cfe-commits]

lenary marked 3 inline comments as done.
lenary added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:104
+  // so that we don't insert `fp` manipulation code into functions that do not
+  // require it.
+  const MachineFrameInfo  = MF.getFrameInfo();

jrtc27 wrote:
> Shouldn't this all be done by the generic stack realignment code like any 
> other allocation? Or is the issue because it's _register spills_ not explicit 
> allocas?
Yeah the issue is because it’s register spills. I have a nice long commit 
message I wrote that I should update the summary with.

Comment updated nonetheless



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.h:40
 
+  bool requiresVirtualBaseRegisters(const MachineFunction ) const override {
+return true;

I forgot this was left in after some experimentation I did. Will remove it in 
the next update. 



Comment at: llvm/test/CodeGen/RISCV/callee-saved-fpr32s.ll:26
 define void @callee() nounwind {
+; ILP32-ILP32E-LP64-LABEL: callee:
+; ILP32-ILP32E-LP64:   # %bb.0:

These check lines are left over from before. will remove



Comment at: llvm/test/CodeGen/RISCV/stack-realignment.ll:3
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN:   | FileCheck %s -check-prefixes=RV32I,RV32-ILP32
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -verify-machineinstrs < %s \

jrtc27 wrote:
> Multiple prefixes is a bad idea with update_llc_test_checks.py, and why is 
> this one done differently from the rest?
It also doesn’t help to avoid duplication here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via cfe-commits]

lenary added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1525
   unsigned TwoXLenInBytes = (2 * XLen) / 8;
   if (!IsFixed && ArgFlags.getOrigAlign() == TwoXLenInBytes &&
   DL.getTypeAllocSize(OrigTy) == TwoXLenInBytes) {

lenary wrote:
> shiva0217 wrote:
> > lenary wrote:
> > > shiva0217 wrote:
> > > > The variadic argument for ilp32e doesn't need to align to even 
> > > > register. We could also add a test line in vararg.ll.
> > > I'm not sure I agree with this interpretation of the psABI. The [[ 
> > > https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#ilp32e-calling-convention
> > >  | ILP32E Section ]] makes no exception for variadic arguments, and the 
> > > base calling convention is only defined in relation to `XLEN`, not in 
> > > terms of stack alignment.
> > > 
> > > I will add a test to `vararg.ll` so the behaviour is at least tested. 
> > It seems to be the current GCC behavior and the following case could 
> > observe that double will not align to even pair.
> >   #include 
> >   void va_double (int n, ...) {
> > va_list args;
> > va_start (args, n);
> > if (va_arg (args, double) != 2.0)
> >   abort ();
> > va_end (args);
> >}
> >int main (int a) {
> > va_double (1, 2.0);
> > return a;
> >}
> > 
> > In a second thought, it seems that non-fixed double arguments may generate 
> > incorrect code, even with align even pair.
> > For ilp32 or lp64 ABI with feature D, stack alignment will be 16, so even 
> > pair can make sure when pushing/popping the non-fixed double to/from the 
> > stack, it will be 8-byte alignment. For ilp32e with 4-byte alignment, even 
> > pair can not guarantee the double will be pushed to stack with 8-byte 
> > alignment.
> Ah, I see the issue.
> 
> It's not clear that choosing to spill to a register pair where the first 
> register is a multiple of 4 would solve the problem either, right? The 
> problem is that we actually need to realign the spill slots for these 
> register pairs.
> 
> I'm not sure how we achieve this. I will investigate further.
I missed that I need to cover this case. 

I'm going to upload a testcase based on your example, but I'm not quite 
convinced it's correct. It does seem to align the stack correctly for the fp64, 
but that's maybe not the right thing to be doing here?

I haven't managed to execute the assembly in the testcase, but I thought adding 
the testcase was important.



Comment at: llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll:11
+; This test currently fails because the machine code does not validate:
+; RUN: not llc -mtriple=riscv32 -mattr=+d -target-abi ilp32e 
-verify-machineinstrs < %s
+; It will need FileCheck %s -check-prefix=ILP32-LP64-NO-D

shiva0217 wrote:
> lenary wrote:
> > lenary wrote:
> > > shiva0217 wrote:
> > > > shiva0217 wrote:
> > > > > Jim wrote:
> > > > > > shiva0217 wrote:
> > > > > > > lenary wrote:
> > > > > > > > @shiva0217 I think this test is failing because of the base 
> > > > > > > > pointer patch, but I'm not sure. Can you look at the issue? It 
> > > > > > > > thinks that x8 gets killed by a store (which I don't think 
> > > > > > > > should be using x8), and therefore x8 is not live when we come 
> > > > > > > > to the epilog. It's a super confusing issue.
> > > > > > > Hi @lenary, it seems that hasBP() return false in this case, the 
> > > > > > > issue trigger by register allocation allocating x8 which should 
> > > > > > > be preserved. I'm not sure why it will happen, I try to write a 
> > > > > > > simple C code to reproduce the case but fail to do that. Could 
> > > > > > > you obtain the C code for the test case?
> > > > > > It seems that RISCVRegisterInfo::getReservedRegs doesn't add x8(fp) 
> > > > > > into reserved registers (TFI->hasFP(MF) return false), then x8 is a 
> > > > > > candidate register for register allocation. After register 
> > > > > > allocation, some of fpr64 splitted into stack that makes stack need 
> > > > > > to be realign (MaxAlignment(8) > StackAlignment(4)), therefore x8 
> > > > > > should be used as frame pointer (TFI->hasFP(MF) return true). In 
> > > > > > emitting epilogue, instructions for fp adjustment is inserted.
> > > > > With the investigation from @Jim, here is the simple C could 
> > > > > reproduce the case.
> > > > >   extern double var;
> > > > >   extern void callee();
> > > > >   void test(){
> > > > > double val = var;
> > > > > callee();
> > > > > var = val;
> > > > >   }
> > > > > Thanks, @Jim 
> > > > There're might be few ways to fix the issue:
> > > > 1. hasFP() return true for ilp32e ABI with feature D
> > > > 2. hasFP() return true for ilp32e ABI with feature D and there's is a 
> > > > virtual register with f64 type.
> > > > 3. Not allow  ilp32e ABI with feature D.
> > > > Given that most of the targets supported double float instructions have 
> > > > stack alignment at 

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10323
+   bool EABI)
+  : DefaultABIInfo(CGT), XLen(XLen), FLen(FLen) {
+if (EABI)

I think it'd be better to have a `NumArgGPRs(EAABI ? 6 : 8)` here as having a 
default value that gets overwritten is more error-prone (and harder to follow).



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2338
+// the ILP32E ABI.
+static const MCPhysReg ArgGPRs_NonE[] = {RISCV::X10, RISCV::X11, RISCV::X12,
+ RISCV::X13, RISCV::X14, RISCV::X15,

Underscores with camel-case isn't great. Maybe ArgIGPRs and ArgEGPRs or similar?



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:104
+  // so that we don't insert `fp` manipulation code into functions that do not
+  // require it.
+  const MachineFrameInfo  = MF.getFrameInfo();

Shouldn't this all be done by the generic stack realignment code like any other 
allocation? Or is the issue because it's _register spills_ not explicit allocas?



Comment at: llvm/test/CodeGen/RISCV/stack-realignment.ll:3
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN:   | FileCheck %s -check-prefixes=RV32I,RV32-ILP32
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -verify-machineinstrs < %s \

Multiple prefixes is a bad idea with update_llc_test_checks.py, and why is this 
one done differently from the rest?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Jim Lin via Phabricator via cfe-commits]

Jim added inline comments.



Comment at: llvm/test/CodeGen/RISCV/calling-conv-ilp32e-double-bug.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -mattr=+f -verify-machineinstrs 
< %s
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -mattr=+f,+d 
-verify-machineinstrs < %s

No FileCheck line here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via cfe-commits]

lenary marked 3 inline comments as done.
lenary added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1525
   unsigned TwoXLenInBytes = (2 * XLen) / 8;
   if (!IsFixed && ArgFlags.getOrigAlign() == TwoXLenInBytes &&
   DL.getTypeAllocSize(OrigTy) == TwoXLenInBytes) {

shiva0217 wrote:
> lenary wrote:
> > shiva0217 wrote:
> > > The variadic argument for ilp32e doesn't need to align to even register. 
> > > We could also add a test line in vararg.ll.
> > I'm not sure I agree with this interpretation of the psABI. The [[ 
> > https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#ilp32e-calling-convention
> >  | ILP32E Section ]] makes no exception for variadic arguments, and the 
> > base calling convention is only defined in relation to `XLEN`, not in terms 
> > of stack alignment.
> > 
> > I will add a test to `vararg.ll` so the behaviour is at least tested. 
> It seems to be the current GCC behavior and the following case could observe 
> that double will not align to even pair.
>   #include 
>   void va_double (int n, ...) {
> va_list args;
> va_start (args, n);
> if (va_arg (args, double) != 2.0)
>   abort ();
> va_end (args);
>}
>int main (int a) {
> va_double (1, 2.0);
> return a;
>}
> 
> In a second thought, it seems that non-fixed double arguments may generate 
> incorrect code, even with align even pair.
> For ilp32 or lp64 ABI with feature D, stack alignment will be 16, so even 
> pair can make sure when pushing/popping the non-fixed double to/from the 
> stack, it will be 8-byte alignment. For ilp32e with 4-byte alignment, even 
> pair can not guarantee the double will be pushed to stack with 8-byte 
> alignment.
Ah, I see the issue.

It's not clear that choosing to spill to a register pair where the first 
register is a multiple of 4 would solve the problem either, right? The problem 
is that we actually need to realign the spill slots for these register pairs.

I'm not sure how we achieve this. I will investigate further.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Shiva Chen via Phabricator via cfe-commits]

shiva0217 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1525
   unsigned TwoXLenInBytes = (2 * XLen) / 8;
   if (!IsFixed && ArgFlags.getOrigAlign() == TwoXLenInBytes &&
   DL.getTypeAllocSize(OrigTy) == TwoXLenInBytes) {

lenary wrote:
> shiva0217 wrote:
> > The variadic argument for ilp32e doesn't need to align to even register. We 
> > could also add a test line in vararg.ll.
> I'm not sure I agree with this interpretation of the psABI. The [[ 
> https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#ilp32e-calling-convention
>  | ILP32E Section ]] makes no exception for variadic arguments, and the base 
> calling convention is only defined in relation to `XLEN`, not in terms of 
> stack alignment.
> 
> I will add a test to `vararg.ll` so the behaviour is at least tested. 
It seems to be the current GCC behavior and the following case could observe 
that double will not align to even pair.
  #include 
  void va_double (int n, ...) {
va_list args;
va_start (args, n);
if (va_arg (args, double) != 2.0)
  abort ();
va_end (args);
   }
   int main (int a) {
va_double (1, 2.0);
return a;
   }

In a second thought, it seems that non-fixed double arguments may generate 
incorrect code, even with align even pair.
For ilp32 or lp64 ABI with feature D, stack alignment will be 16, so even pair 
can make sure when pushing/popping the non-fixed double to/from the stack, it 
will be 8-byte alignment. For ilp32e with 4-byte alignment, even pair can not 
guarantee the double will be pushed to stack with 8-byte alignment.



Comment at: llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll:11
+; This test currently fails because the machine code does not validate:
+; RUN: not llc -mtriple=riscv32 -mattr=+d -target-abi ilp32e 
-verify-machineinstrs < %s
+; It will need FileCheck %s -check-prefix=ILP32-LP64-NO-D

lenary wrote:
> lenary wrote:
> > shiva0217 wrote:
> > > shiva0217 wrote:
> > > > Jim wrote:
> > > > > shiva0217 wrote:
> > > > > > lenary wrote:
> > > > > > > @shiva0217 I think this test is failing because of the base 
> > > > > > > pointer patch, but I'm not sure. Can you look at the issue? It 
> > > > > > > thinks that x8 gets killed by a store (which I don't think should 
> > > > > > > be using x8), and therefore x8 is not live when we come to the 
> > > > > > > epilog. It's a super confusing issue.
> > > > > > Hi @lenary, it seems that hasBP() return false in this case, the 
> > > > > > issue trigger by register allocation allocating x8 which should be 
> > > > > > preserved. I'm not sure why it will happen, I try to write a simple 
> > > > > > C code to reproduce the case but fail to do that. Could you obtain 
> > > > > > the C code for the test case?
> > > > > It seems that RISCVRegisterInfo::getReservedRegs doesn't add x8(fp) 
> > > > > into reserved registers (TFI->hasFP(MF) return false), then x8 is a 
> > > > > candidate register for register allocation. After register 
> > > > > allocation, some of fpr64 splitted into stack that makes stack need 
> > > > > to be realign (MaxAlignment(8) > StackAlignment(4)), therefore x8 
> > > > > should be used as frame pointer (TFI->hasFP(MF) return true). In 
> > > > > emitting epilogue, instructions for fp adjustment is inserted.
> > > > With the investigation from @Jim, here is the simple C could reproduce 
> > > > the case.
> > > >   extern double var;
> > > >   extern void callee();
> > > >   void test(){
> > > > double val = var;
> > > > callee();
> > > > var = val;
> > > >   }
> > > > Thanks, @Jim 
> > > There're might be few ways to fix the issue:
> > > 1. hasFP() return true for ilp32e ABI with feature D
> > > 2. hasFP() return true for ilp32e ABI with feature D and there's is a 
> > > virtual register with f64 type.
> > > 3. Not allow  ilp32e ABI with feature D.
> > > Given that most of the targets supported double float instructions have 
> > > stack alignment at least eight bytes to avoid frequently realignment. 
> > > Would it more reasonable to have a new embedded ABI with stack alignment 
> > > at least eight bytes to support feature D?
> > @Jim, @shiva0217, thank you very much for tracking down this bug, and 
> > providing a small testcase, that's very helpful.
> > 
> > We talked about this on the call this week, and I indicated I was going to 
> > go with a solution as close to 2 as I could.
> > 
> > I have since started an investigation (which I hoped would be quicker than 
> > it is) of what happens if we implement `canRealignStackFrame` to check if 
> > FP is unused, and this also seems to solve the problem. I'm doing some 
> > deeper checks (which require implementing parts of the backend around MIR 
> > that I haven't looked at before), but I think this might be a better 
> > solution? I'll keep this patch updated on when I upload the fix for stack 
> > realignment to cover this case. In the 

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Luís Marques via Phabricator via cfe-commits]

luismarques added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVCallingConv.td:36
+// The only physical register that isn't saved is x2 (SP), which is used by the
+// processor when the interrupt happens.
+

Nitpick: "the interrupt happens" -> "an interrupt happens" (or, even better, 
"is serviced").



Comment at: llvm/test/CodeGen/RISCV/calling-conv-ilp32e-double-bug.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -mattr=+f -verify-machineinstrs 
< %s

It would be good to describe in a comment what the bug is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via cfe-commits]

lenary added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1525
   unsigned TwoXLenInBytes = (2 * XLen) / 8;
   if (!IsFixed && ArgFlags.getOrigAlign() == TwoXLenInBytes &&
   DL.getTypeAllocSize(OrigTy) == TwoXLenInBytes) {

shiva0217 wrote:
> The variadic argument for ilp32e doesn't need to align to even register. We 
> could also add a test line in vararg.ll.
I'm not sure I agree with this interpretation of the psABI. The [[ 
https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#ilp32e-calling-convention
 | ILP32E Section ]] makes no exception for variadic arguments, and the base 
calling convention is only defined in relation to `XLEN`, not in terms of stack 
alignment.

I will add a test to `vararg.ll` so the behaviour is at least tested. 



Comment at: llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll:11
+; This test currently fails because the machine code does not validate:
+; RUN: not llc -mtriple=riscv32 -mattr=+d -target-abi ilp32e 
-verify-machineinstrs < %s
+; It will need FileCheck %s -check-prefix=ILP32-LP64-NO-D

lenary wrote:
> shiva0217 wrote:
> > shiva0217 wrote:
> > > Jim wrote:
> > > > shiva0217 wrote:
> > > > > lenary wrote:
> > > > > > @shiva0217 I think this test is failing because of the base pointer 
> > > > > > patch, but I'm not sure. Can you look at the issue? It thinks that 
> > > > > > x8 gets killed by a store (which I don't think should be using x8), 
> > > > > > and therefore x8 is not live when we come to the epilog. It's a 
> > > > > > super confusing issue.
> > > > > Hi @lenary, it seems that hasBP() return false in this case, the 
> > > > > issue trigger by register allocation allocating x8 which should be 
> > > > > preserved. I'm not sure why it will happen, I try to write a simple C 
> > > > > code to reproduce the case but fail to do that. Could you obtain the 
> > > > > C code for the test case?
> > > > It seems that RISCVRegisterInfo::getReservedRegs doesn't add x8(fp) 
> > > > into reserved registers (TFI->hasFP(MF) return false), then x8 is a 
> > > > candidate register for register allocation. After register allocation, 
> > > > some of fpr64 splitted into stack that makes stack need to be realign 
> > > > (MaxAlignment(8) > StackAlignment(4)), therefore x8 should be used as 
> > > > frame pointer (TFI->hasFP(MF) return true). In emitting epilogue, 
> > > > instructions for fp adjustment is inserted.
> > > With the investigation from @Jim, here is the simple C could reproduce 
> > > the case.
> > >   extern double var;
> > >   extern void callee();
> > >   void test(){
> > > double val = var;
> > > callee();
> > > var = val;
> > >   }
> > > Thanks, @Jim 
> > There're might be few ways to fix the issue:
> > 1. hasFP() return true for ilp32e ABI with feature D
> > 2. hasFP() return true for ilp32e ABI with feature D and there's is a 
> > virtual register with f64 type.
> > 3. Not allow  ilp32e ABI with feature D.
> > Given that most of the targets supported double float instructions have 
> > stack alignment at least eight bytes to avoid frequently realignment. Would 
> > it more reasonable to have a new embedded ABI with stack alignment at least 
> > eight bytes to support feature D?
> @Jim, @shiva0217, thank you very much for tracking down this bug, and 
> providing a small testcase, that's very helpful.
> 
> We talked about this on the call this week, and I indicated I was going to go 
> with a solution as close to 2 as I could.
> 
> I have since started an investigation (which I hoped would be quicker than it 
> is) of what happens if we implement `canRealignStackFrame` to check if FP is 
> unused, and this also seems to solve the problem. I'm doing some deeper 
> checks (which require implementing parts of the backend around MIR that I 
> haven't looked at before), but I think this might be a better solution? I'll 
> keep this patch updated on when I upload the fix for stack realignment to 
> cover this case. In the case that this fix isn't enough, I'll look to 
> implement solution 2.
> 
> In any case, it's evident that allocating a spill slot for a register that 
> has higher spill alignment than the stack slot, is the kernel of the problem, 
> and this may arise again depending on how we choose to implement other 
> extensions.
> 
> 
I couldn't find a reasonable way to check for a virtual (or physical) register 
of type fp64, without iterating over all the instructions in a function, which 
I'd prefer not to do.

So Instead I have implemented option 1 in `hasFP`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70401/new/

https://reviews.llvm.org/D70401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits