LGTM. Thanks @HsiangKai.

LGTM

My question is how to count the scalar incoming argument with this frame layout,
because scalar incoming arguments cross RVV previous local variables frame objects.
In the current frame(callee), we don't know the RVV previous local variables count.
So we can't get the real stack offset because callee not know the caller vector local variables size.

One could argue that this introduces coupling between the scalar register bank and the vector register bank. But I presume the simpler scalar load makes up for that loss of decoupling.

I was under the impression we didn't want to use class-member access syntax for vector tuples (see https://github.com/riscv/rvv-intrinsic-doc/issues/17#issuecomment-628998077 ) so we don't need a record type, do we?

This is just a suggestion, feel free to ignore: a sequence of Ts was easy to parse for the prototype but may be want to consider something like T3v rather than TTTv. I think it would simplify the TString tblgen class and those std::string(N, 'T').

Hi @SuH wrote:

I have a question about access incoming args.

ChangeLog:

• Rebase

In D98801#2644290, @frasercrmck wrote:

LGTM though (sorry) maybe the tests could have a short comment explaining what they're testing (since they'll be fixed by a future patch anyway) and what to be wary of when the output changes.

ChangeLog:

• Describe a bit better the purpose of the test.

I have internally validated that the last change (Diff 331637, not Diff 332358 that only adds a comment) is equivalent to the previous change (Diff 331546).

ChangeLog:

• Add comment explaining why we add the alignment of the stack as RVV padding when CSR are not aligned to 8 bytes.

ChangeLog:

• Clarify the purpose of the change.
• Adjust syntax of the IR input.

ChangeLog:

• Fold the padding into the main stack size enlargement: adjust CSR offsets accordingly, as suggested by @StephenFan
• Add {get,set}RVVPadding to RISCVMachineFunctionInfo to materialise the extra padding required by RVV.

In D98802#2634791, @StephenFan wrote:

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.

I feel this might a bit complicated to grasp from my attempt of explanation in the summary, so this is how the stack should look like for wrong-stack-slot-rv32.mir assuming VLENB=128.

ChangeLog:

• Add extra padding so we can adjust RVV objects within the stack correctly
• Round the offset to the RVV objects to 8 bytes (when not using fp).
• Move computation of CalleeSavedStackSize from processFunctionBeforeFrameIndicesReplaced to processFunctionBeforeFrameFinalized because we need that size before we emit the prologue.

ChangeLog:

• Simplify the MIR input
• Split test in two for RV32 and RV64

Hum, this is not right. I just realized that we may need some extra padding between the end of the scalar part and the RVV stack slots. That padding hasn't been accounted anywhere.

Overall LGTM. Thanks @khchen!

Hi. We did this out of an abundance of caution. We wanted to be sure after the vsetvli were emitted, we didn't attempt to use the SEW operand (either by accident or intentionally).

Except for the minor comments above, LGTM.

Yes, I referenced this out-of-reach-emergency-slot.mir test. But with a stack object that has a out-of-range size. And I think my use-emergency-spill-slot.mir test and out-of-reach-emergency-slot.mir test are a little repeated. So should I keep my use-emergency-spill-slot.mir test or delete it?

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?

Can you add some tests for the cases with overaligned stuff and variable sized + rvv?

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

Thanks @HsiangKai. LGTM.

I'm happy with the patch as it stands now. At least it gives us a baseline to improve if that is needed.

Hi Sander, one question OptionalVFCandidates::addMaxVF combines using the maximum but AFAICT it is not used in a loop. Maybe you plan to "update" the corresponding VF values (scalable and/or fixed) in other places (not shown in the current patch, yet)?

I understand it is not a win to apply a similar scheme to the base RISCVInstrInfo.td and perhaps F's and D's because we do have an offset available there. Is my understanding correct?

I admit that I initially wasn't convinced by the idea of ComplexPattern including both the matching logic (which is only return true) and a bit of selection logic (selectImm) in the input DAG. But on a second thought this allows the two logics be kept together.

Hi Kai, are we OK with having a test that goes from IR to assembly in the Transforms component?

LGTM. Thanks @craig.topper

We're multiplying by 3 the _VF pseudos but to be honest I don't see any better alternative.

Ping.

This seems a very sensible approach.

I prefer this approach. Computing the address is a bit more involved but doesn't need an extra memory access.

Hi Serge,

This is a very nice cleanup. Thanks @craig.topper!

ChangeLog:

• Largely simplify the testcase by making a call that uses all the eligible registers forcing the RegisterScavenger to use the emergency spill.

ChangeLog:

• Rebase patch prior committing

Hi Serge, would it make sense to use a Pseudo for those specific cases with a custom inserter? (usesCustomInserter = 1 in the tablegen definition of the Pseudo)

For instructions that use a uimm5 immediate, this change only affects
constants >= 128 for i8 or >= 32768 for i16. Constants that large
already wouldn't have been eligible for uimm5 and would need to use a
scalar register.

Agreed, these instructions and their intrinsics seems pretty uncontroversial. LGTM.

Also LGTM once Craig's comments are addressed. Thanks a lot Kai!

This looks good to me. Thanks a lot @craig.topper

As a first step to remove the most obviously redundant vsetvli cases, this LGTM.

I don't know if we're going to have an answer for the masked/unmasked soon. I think we might just use earlyclobber in the initial patches and suffer the bad register allocation so we can make forward progress.

Hi @simoll: a quick question regarding vp.load/vp.store/vp.gather/vp.scatter. Does the current definition of VPred allow for something similar to the !nontemporal metadata of regular load/store instructions? I don't see any explicit mention to that but maybe it is already possible using metadata or some other annotation?

Hi all,

Hi Andrey, I can confirm this fixes the build for RISC-V.

Hi Todd,

Hi Todd,

Random number won't work, because the goal of the library registering is to catch several libraries in a single process. So a process should have deterministic file name (or environment variable name).

[Drive by] What about appending the process id or a random number to the file name, or both?

Hi Todd,

Hi, one question about this change: