Hi Ram,

LGTM

In D152279#4422887, @MaskRay wrote:

In D152279#4420312, @asb wrote:

In D152279#4415974, @MaskRay wrote:

However, RISC-V -msmall-data-limit= is probably a case warranting a difference.
The global pointer relaxation has a very limited value (benchmarked by multiple parties, including a party which implemented this feature in the GNU toolchain: even they can only say the optimization only applies to very specific projects).
The default value is confusing: as explained by the summary.

I suspect that -msmall-data-limit=8 is too conservative, maybe 16 would be better, I don't know. I think global pointer relaxation users should toggle this by themselves, not relying on 0 or 8 default decided by a bunch of strange conditions.

I don't disagree that the small data limit being 8 rather than something else doesn't seem to be particularly well motivated, but I understand that the case where the option does make a difference is on embedded targets (I think the data that was shared before was for SPEC, but could be wrong?). I think we can generally expect more willingness for people targeting embedded systems to explore different compiler flags,

Agree.

but just matching gcc does feel like a better default. What do you think about keeping the default for bare metal targets?

I am unsure we want to add the complexity and refrain from changing the default for bare metal targets due to haunted graveyards https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf

We are already different from GCC in a number of ways:

• for -fpie, we may emit .sdata/.sbss but GCC won't
• we accept -G while GCC doesn't.
• we don't emit .srodata.

I made a comment on https://lists.riscv.org/g/sig-toolchains/message/619 and informed GCC folks in case they can change the default as well.
If they don't, I think we made a good choice for not following this particular behavior.

(I feel sad that I did not see D57497 and it landed with a blocked review. If we started from scratch, we would probably run into a cleaner state.)

In D150671#4353168, @artagnon wrote:

Include test, from a benchmark run reduced by hand.

I know the test isn't pretty, but it's the best I could do within reasonable time.

Theoretically, NewRootHeight and OldRootHeight should be equal.
Does it make sense to remove the Heights calculations?

In D150192#4330199, @efriedma wrote:

From what I recall, "inrange" is actually more restrictive than the normal C/C++ array indexing rules. Specifically, the bits regarding comparisons. "inrange" was designed to allow splitting globals indexed using inrange.

That isn't to say that functionality like this isn't useful, but we probably need to call it something different. And I'm not sure an attribute on a GEP is actually the best way to represent the semantics. It might make sense to use a separate intrinsic to represent this, especially given the direction of https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699 .

In D143248#4177761, @varunkumare99 wrote:

In D143248#4131555, @shiva0217 wrote:

Hi Varun,

It might be good to have a simple case to illustrate that cfi_remember_state and cfi_restore_state can be generated properly with epilogue CFI.
For examle, could the following case generated with cfi_remember_state and cfi_restore_state as GCC?

extern void foo(void);
void f(int *i) {
  if (*i == 0) {
    *i = 1;
    foo();
    return;
  } else {
    __builtin_printf("Hi");
    *i=0;
  }
}

Hi Shiva, the following is the assembly emitted.

f:
.Lfunc_begin0:
	.loc	0 2 0
	.cfi_startproc
	addi	sp, sp, -16
	.cfi_def_cfa_offset 16
	sd	ra, 8(sp)
	sd	s0, 0(sp)
	.cfi_offset ra, -8
	.cfi_offset s0, -16
	.cfi_remember_state
	mv	s0, a0
.Ltmp0:
	.loc	0 3 6 prologue_end
	lw	a0, 0(a0)
.Ltmp1:
	.loc	0 3 6 is_stmt 0
	beqz	a0, .LBB0_2
.Ltmp2:
	.loc	0 8 3 is_stmt 1
	lui	a0, %hi(.L.str)
	addi	a0, a0, %lo(.L.str)
	call	printf
.Ltmp3:
	.loc	0 9 5
	sw	zero, 0(s0)
.Ltmp4:
	.loc	0 11 1
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp5:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 11 1 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
	ret
.LBB0_2:
	.cfi_restore_state
.Ltmp6:
	.loc	0 0 1
	li	a0, 1
.Ltmp7:
	.loc	0 4 6 is_stmt 1
	sw	a0, 0(s0)
	.loc	0 5 3
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp8:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 5 3 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
.Ltmp9:
	tail	foo
.Ltmp10:
.Lfunc_end0:
	.size	f, .Lfunc_end0-f
	.cfi_endproc

Hi Varun,

In D79378#4103366, @rjmccall wrote:

In D79378#4101935, @shiva0217 wrote:

In D79378#4101829, @rjmccall wrote:

In D79378#4101613, @shiva0217 wrote:

Hi,

I have a question for the delete function call sinking in -Oz.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf.
According to 3.7.4.2/3
The value of the first argument supplied to a deallocation function may be a null pointer value; if so, and if the deallocation function is one supplied in the standard library, the call has no effect. Otherwise, the behavior is undefined

It seems the null checking can be skipped only when the delete is not from the standard library(have been overloaded by the user), isn't it?

The paragraph you just quoted says that the deallocation functions provided by the standard library are required have no effect on null pointers. That means it's fine to call them with a null pointer even if the source wouldn't normally execute that call. We could just insert these calls into random other code if we wanted to.

Is a null operand of delete in the source code no effect because there will be null pointer checking generated? Or the delete(null) in assembly code also valid?

In [expr.delete], the standard specifies that using the delete operator on a null pointer is required to have no effect. It does not specify how that should be implemented. In [basic.std.dynamic.deallocation], the standard also specifies that the library-provided operator delete functions must have no effect when given a null pointer. This requirement applies no matter how the function is called, so yes, a call from assembly that passed a null pointer would also be required to have no effect. More officially, you can simply take the address of operator delete, yielding an rvalue of void (*)(void *) (or similar, depending on which operator delete you use), which can then be called later in a context that doesn't understand it's calling a deallocation function; such a call is still required to have no effect when passing a null pointer, because the requirement is laid on the function, separate from the circumstances of it being called as part of the delete operator.

Note also that calling operator delete unconditionally when implementing the delete operator is explicitly blessed by [expr.delete]p7:

If the value of the operand of the delete-expression is not a null pointer value, then:

...

Otherwise, it is unspecified whether the deallocation function will be called.

In D79378#4101829, @rjmccall wrote:

In D79378#4101613, @shiva0217 wrote:

Hi,

I have a question for the delete function call sinking in -Oz.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf.
According to 3.7.4.2/3
The value of the first argument supplied to a deallocation function may be a null pointer value; if so, and if the deallocation function is one supplied in the standard library, the call has no effect. Otherwise, the behavior is undefined

It seems the null checking can be skipped only when the delete is not from the standard library(have been overloaded by the user), isn't it?

The paragraph you just quoted says that the deallocation functions provided by the standard library are required have no effect on null pointers. That means it's fine to call them with a null pointer even if the source wouldn't normally execute that call. We could just insert these calls into random other code if we wanted to.

I have a question for the delete function call sinking in -Oz.

In D28638#3710423, @llozano wrote:

Did anything happen with these NDS32 patches?
Is there any effort to add support for it?

In D79141#2011973, @lenary wrote:

Update after shiva0217's review:

• Update test to correctly show splitting on RV32*C
• Use early return if CSI.size() == 0
• Update comments to count bits correctly.

I decided against a getOffsetAddressableLimit function, as the logic in
getFirstSPAdjustAmount isn't just about addressable offsets, it's also about
what we can adjust in a single (compressed if possible) instruction, so I
thought that adding that function would make the logic harder to understand.

Got your point. The logic is not only for addressable but also for preference for code size.

Although most of the targets declare these functions as private, I think it might worth changing to public. So the various downstream RISCV users may able to inherit the class and customize the functions for their ISA and return the basic class functions to utilize the original infrastructure. Any thoughts?

In D77117#1952929, @lenary wrote:

@shiva0217 yeah, that sounds good. Please can you ensure you include some of that explanation in the commit message when you land this?

In D77117#1952051, @lenary wrote:

Hi @shiva0217 can you clarify as to why this is necessary?

Update patch to address @apazos's comments.

In D57497#1927881, @apazos wrote:

Shiva, how about making the flag small-data-limit alias of -msmall-data-threshold?

In D57497#1921497, @lenary wrote:

In D57497#1920870, @shiva0217 wrote:

In D57497#1920097, @apazos wrote:

Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine.

I see there is a msmall-data-threshold flag used by Mips and Hexagon, and now we are adding a new flag msmall-data-limit. Should't we reuse the flag?

Hi Ana,
Thanks for trying the patch. msmall-data-limit is also a RISCV GCC flag, so I would recommend using the same flag as GCC.

How hard would it be to use the -msmall-data-threshold flag work if a -msmall-data-limit flag is not provided?

In D57497#1920097, @apazos wrote:

Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine.

I see there is a msmall-data-threshold flag used by Mips and Hexagon, and now we are adding a new flag msmall-data-limit. Should't we reuse the flag?

Update the patch to address @apazos's comments.

In D57497#1917835, @apazos wrote:

Shiva, I see a warning always being printed:

'+small-data-limit=' is not a recognized feature for this target (ignoring feature)

This is because it is being passed down as a target feature.

Might be good to add a test case to make sure the SmallDataLimit module flag is created, no target feature is passed, and that no warnings are printed.

Update patch to address @jrtc27's comment.

Update patch to address @evandro's comments.

Update patch to address @lenary's comments.

Rebase to the trunk

In D57497#1913473, @apazos wrote:

Shiva, we forgot about this patch. Can you rebase it so we move on with merging.

Hi @HsiangKai,
Thanks for the review.

Update the patch to address @HsiangKai's comments.

The patch has landed with https://github.com/llvm/llvm-project/commit/1cae2f9d192c69833e22684ca338660942ab464e. I forgot to add the "Differential Revision:" in the commit message, so I closed the revision manually. Sorry for the inconvenience.

Hi @lenary and @luismarques,
Thanks for the review.

Update the patch to fix the typo.

Update the patch to address the comment.

LGTM, Labels required to describe call site scope will be generated for the calls after FrameSetup instructions. When the SaveLibcall mark as FrameSetup, the Label will not be generated, so constructing call site entry for SaveLibcall will trigger an assertion. Given that SaveLibcall should be part of the prologue, so I think generating call site begin after FrameSetup instructions should be reasonable. But I hope there could be a second look.

With the patch, double load/store instructions may unaligned-access with -mabi=ilp32e -mattr=+d flags. Could the load/store support unaligned-access?

Rebase to the master

Hi @luismarques,
I tried to generate .cfi_remember_state and .cfi_restore_state in emitEpilogue. It could pass the test case.
But there is an issue that .cfi_restore_state will become incorrect if the shrink wrapping hoists the epilogue.
So I think to remove CFI directives from the epilogue as other targets do might be a reasonable step.

Hi @luismarques, thanks for the patch, LGTM.

Hi @luismarques, thanks for fixing this.

Add CFI checking line in the test case

Update patch to address the comments.

In D68979#1713763, @apazos wrote:

Shiva, I have tried a few workloads like EEMBC, SPEC2000/2006, perennial c++, plumhall.
They don't seem to use variable length arrays nor allocas to test this patch.
Which test suite are you using?

Rebase the test case.

In D68559#1699606, @lenary wrote:

I like this change.

fastcc is LLVM-internal only, right? Checking that we don't have to care about splitting operands that don't fit into registers, or any other psABI details, right?

Yes, to my understanding, fastcc doesn't need to care about psABI details.

Update patch to address the feedbacks

Update patch to address the comment

Fix comment in the patch.

Add splitting SP adjustment boundary test case as @luismarques suggest.

In D68011#1683996, @luismarques wrote:

In D68011#1683772, @shiva0217 wrote:

Update patch

@shiva0217 Thanks for updating the patch and addressing the previous concerns. This is looking quite good. I have just a couple more concerns before approving this:

• The boundary condition for doing the two-stage sp adjustment probably needs adjustment. For instance, try this:

int main() {
    char xx[2048-16];
    foo(xx);
}
--
        addi    sp, sp, -2032
        sd      ra, 2024(sp)
        addi    sp, sp, -16
...

I think that one would still fit a single-stage sp update. Related to that, where you have the code if (!isInt<12>(StackSize) && (CSI.size() > 0)), be sure that the isInt<12> check properly accounts for the StackSize, CSI size, StackAlign etc. Which brings me to the second point...

• Please include tests showing that the boundary condition is correctly computed. For instance, one test where the stack size is just enough not to have to split the sp adjustment and another where the adjustment is needed.

Thanks!

Update patch

Update patch

1. Call getFirstSPAdjustAmount() once in per prologue/epilogue generation as @luismarques suggest
2. Generate CFI directives in large-stack.ll test case to check the CFI generation

Update patch to address the comments.

In D67698#1675370, @lenary wrote:

We discussed this in the RISC-V meeting on 19 Sep 2019.

• Pros: GCC for RISC-V and LLVM for ARM and AArch64 seem to do the same. It can help in situations with particularly bad register pressure requirements.
• Cons: It can make debugging a lot harder, though this seems not to be an issue in GDB for RISC-V.

I think we don't want to have yet another configuration flag to control this.

It would be good to see some performance comparison, but I realise you may not be able to release internal benchmarks, and we have no public benchmarking system for RISC-V.