In D98101#2663432, @asb wrote:

Could you please rebase this to account for D98716?

I _think_ the right fix is to change the call for needsStackRealignment to shouldRealignStack. But I'm seeing multiple runtime failures for the GCC torture suite with the patch applied. e.g. 20031012-1.c at O0 for rv32imafdc ilp32.

Fix error.

Ping.

Hi @rogfer01 ! It is reasonable to me. But I think the instruction of BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), SPReg) can be eliminated. Firstly, the value of calleeSavedStackSize can be regarded as a aligned value(For example, aligned to MFI.getStackAlign()). Then we can calculate the padding size by the aligned calleeSavedStackSize minus the original calleeSavedStackSize. When emits prologue, we can minus the value of
MFI.getStackSize() - original calleeSavedStackSize + aligned calleeSavedStackSize. When get the offset of the rvv object, we can MFI.getStackSize() - original calleesavedStackSize, because we just want to calculate the
non-calleesaved field size. My English is poor and I am a beginner of LLVM-RISCV. So I don't know if it makes sense to you.

rebase

Address @rogfer01 's comment.

Fix comment error.

Pre-commit local-stack-allocation.ll test.

Delete white spaces.

Address @rogfer01 's comment. Add rvv-framelayout.ll test.

In D97274#2593601, @rogfer01 wrote:

Isn't this very similar to https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/out-of-reach-emergency-slot.mir except for a missing SD?

Address @luismarques 's comment

delete unnecessary white space

Address @jrtc27 's comments

Ping.

Address @rogfer01 's comments.

address @HsiangKai and @rogfer01 's comments

In D97111#2578131, @HsiangKai wrote:

In D97111#2577248, @StephenFan wrote:

In D97111#2576958, @rogfer01 wrote:

Hi @StephenFan. I wonder if we want to do this only when we index via sp.

My understanding is that the emergency slot will be close enough when we index via fp/bp in the previous layout. In the previous layout, when indexing a scalar via sp we have to cross the RVV part (which needs an extra register) to reach the scalar registers (where the emergency slot will be located). IIUC, your approach it fixes the sp case but complicates the fp/bp case to access the emergency slot because now we need to jump over RVV.

My suggestion means we want to have the RVV vectors as far from the frame pointer as possible (the one being used sp/fp/bp), so scalars are still straightforward to access even in the presence of RVV. This means different layouts when indexing with sp and when indexing with fp/bp. In summary: your new layout for sp but the previous one for fp/bp. Does this make sense?

Make sense to me. I didn't consider the case that needs fp to access the emergency spill slot. In my patch, It seems that make use of the bp register is necessary when variable-sized local variables and rvv stack objects exist at the same time. Because if uses fp to access the emergency spill slot needs an extra register. In the original patch, make use of the fp register is necessary when rvv stack objects exist. I prefer the current frame layout, Because it only needs a bp register when variable-sized local variables and rvv stack objects exist. Do you agree it?

When it is necessary to have fp, the frame layout will look like as below.

|---------------------------------| <- frame pointer (fp)
| scalar callee-saved registers   |
|---------------------------------|
| scalar local variables          |
|---------------------------------|
| ///// realignment /////         |
|---------------------------------|
| scalar outgoing arguments       |
|---------------------------------|
| RVV local variables &&          |
| RVV outgoing arguments          |
|---------------------------------| <- end of frame (sp)

What do you mean "if uses fp to access the emergency spill slot needs an extra register"?

I agree with Roger. We only need to change the frame layout when accessing frame objects only using sp. That is, we should put RVV objects as far as 'base' as possible. The 'base' could be sp, fp or bp.

In D97111#2577248, @StephenFan wrote:

In D97111#2576958, @rogfer01 wrote:

Hi @StephenFan. I wonder if we want to do this only when we index via sp.

My understanding is that the emergency slot will be close enough when we index via fp/bp in the previous layout. In the previous layout, when indexing a scalar via sp we have to cross the RVV part (which needs an extra register) to reach the scalar registers (where the emergency slot will be located). IIUC, your approach it fixes the sp case but complicates the fp/bp case to access the emergency slot because now we need to jump over RVV.

My suggestion means we want to have the RVV vectors as far from the frame pointer as possible (the one being used sp/fp/bp), so scalars are still straightforward to access even in the presence of RVV. This means different layouts when indexing with sp and when indexing with fp/bp. In summary: your new layout for sp but the previous one for fp/bp. Does this make sense?

Make sense to me. I didn't consider the case that needs fp to access the emergency spill slot. In my patch, It seems that make use of the bp register is necessary when variable-sized local variables and rvv stack objects exist at the same time. Because if uses fp to access the emergency spill slot needs an extra register. In the original patch, make use of the fp register is necessary when rvv stack objects exist. I prefer the current frame layout, Because it only needs a bp register when variable-sized local variables and rvv stack objects exist. Do you agree it?

In D97111#2576958, @rogfer01 wrote:

Hi @StephenFan. I wonder if we want to do this only when we index via sp.

My understanding is that the emergency slot will be close enough when we index via fp/bp in the previous layout. In the previous layout, when indexing a scalar via sp we have to cross the RVV part (which needs an extra register) to reach the scalar registers (where the emergency slot will be located). IIUC, your approach it fixes the sp case but complicates the fp/bp case to access the emergency slot because now we need to jump over RVV.

My suggestion means we want to have the RVV vectors as far from the frame pointer as possible (the one being used sp/fp/bp), so scalars are still straightforward to access even in the presence of RVV. This means different layouts when indexing with sp and when indexing with fp/bp. In summary: your new layout for sp but the previous one for fp/bp. Does this make sense?

In D97111#2576382, @StephenFan wrote:

address @criag.topper 's comment

add test cases that deleted incautious

address @criag.topper 's comment

Ping.

Address @craig.topper 's comments

Address craig.topper 's comment

Address craig.topper 's comment

ping.

In D93298#2544775, @asb wrote:

In D93298#2544459, @StephenFan wrote:

According to @jrtc27 's review that is
"As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.".
We split the Zfinx into 3 separate extensions which is Zfinx, Zdinx, and Zfhinx.

Ah I see. I interpreted jrtc27's comment as a general gripe about the spec (which perhaps could be relayed to those working on the zfinx spec) rather as a direction for changing this patch in particular. Anyway, it's a detail that shouldn't affect an initial review. Thanks for clarifying.

In D93298#2544443, @asb wrote:

I started reviewing this alongside the specification in https://github.com/riscv/riscv-zfinx/blob/master/Zfinx_spec.adoc. At the time of writing, it seems to define "zfinx" but not "zfhinx" and "zfdinx" as seem to be used in this patch. I think intent is that rv32ifd_zfinx is the equivalent of "zfdinx" in this patch. Is there a reason to go for different naming, or a different version of the spec I should be looking at?

According to @jrtc27 's review that is
"As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.".
We split the Zfinx into 3 separate extensions which is Zfinx, Zdinx, and Zfhinx.

Address craig.topper's comment.

In D95214#2517303, @craig.topper wrote:

In D95214#2517296, @StephenFan wrote:

In D95214#2517277, @craig.topper wrote:

@StephenFan thank you for the bug report. I had a feeling that converting FPR32 to FPR64 with a SUBREG_TO_REG could cause a problem.

I have a question that why not make the scalar float point as FPR64 (not FPR32) then when encounter the FPR32 or FPR16, use the EXTRACT_SUBREG opcode.

If you look at the current output for the fpr-spill-scalar.ll test added in D95234, you'll see that we would generate a 8 byte spill slot for float and half if we use FPR64. But the instruction to store an 8 byte F register isn't supported without the D extension.

In D95214#2517296, @StephenFan wrote:

In D95214#2517277, @craig.topper wrote:

@StephenFan thank you for the bug report. I had a feeling that converting FPR32 to FPR64 with a SUBREG_TO_REG could cause a problem.

I have a question that why not make the scalar float point as FPR64 (not FPR32) then when encounter the FPR32 or FPR16, use the EXTRACT_SUBREG opcode.

Get it.

In D95214#2517277, @craig.topper wrote:

@StephenFan thank you for the bug report. I had a feeling that converting FPR32 to FPR64 with a SUBREG_TO_REG could cause a problem.

In D95214#2515707, @HsiangKai wrote:

In D95234, I defined different pseudo instructions for different floating-point register classes. The floating-point vector pseudo instructions have correct register class information in D95234. I think it also solves the issue you encountered.

In D95214#2515428, @jrtc27 wrote:

The commit message could do with improving in many respects (all of spelling, grammar, formatting, style and content). Please clearly state what the actual issue is and why this change is correct given the comment that is already there to explain the current behaviour.

ping...

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

In D92479#2428454, @jrtc27 wrote:

It's also wrong if the ADDI has FrameReg as its destination. A neater approach might be to always use the destination register (if there is one, using general instruction query information) as the destination for the ADD, keeping the scratch register for the movImm call. Then you'd only need special logic to work out whether you need to keep the (modified) existing instruction.

In D91931#2410704, @lewis-revill wrote:

In D91931#2410405, @StephenFan wrote:

In D91931#2410204, @craig.topper wrote:

There's already a series of patches to add more GlobalISel support for RISCV. For example, https://reviews.llvm.org/D76445. The others can be found in the stack of patches attached to that one.

I noticed that these patches have been silent for several months, how can I promote this?

I've not been able to work on those patches for some time but if someone might commandeer them to rebase them and work on top of them I think that would be a good starting point and I would happily take a look at the changes. Otherwise I may get some time to update them myself within a month or two.

In D91931#2410204, @craig.topper wrote:

There's already a series of patches to add more GlobalISel support for RISCV. For example, https://reviews.llvm.org/D76445. The others can be found in the stack of patches attached to that one.