[PATCH] D78717: [SystemZ] Implement -fstack-clash-protection

Tue Apr 28 09:06:39 PDT 2020

uweigand added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Not looking at the code so far, but just answering your questions:

> GCC emits an 'lgr %r15,%r1' after the loop, which seems redundant, since it is known that %r15 has the value of %r1 already. Is this required to exist for some reason (omitted by patch for now)?

I agree that this looks redundant.

> gcc seems not to be probing the residual allocation after the loop. However if only two (unrolled) allocations were made, the residual is also probed.

I'm not seeing this, do you have an example?

> I am not aware of any real reason to not simply do the probing directly in emitPrologue(), but it seems wisest to do like X86 since inlineStackProbe() is called from common-code. Perhaps this relates to implementing shrink-wrapping or other things?

The comment you're adding says:

  // stack probing may involve looping, and control flow generations is
  // disallowed at this point. Rely to later processing through
  // `inlineStackProbe`.

That would seem to be the reason why it cannot be done directly inline, right?

> emitBlockAfter(), splitBlockBefore() copied from SystemZISelLowering. Make into SysteZInstrInfo members instead?

Makes sense to me.

>   A little unsure about the use of unsigned vs uint64_t...

Huh.  I'd probably just use uint64_t everywhere.

> I took the X86 tests and copied them over as SystemZ tests and noticed that SystemZ gets these test cases built by SelectionDAGBuilder with dynamic_stackalloc nodes, while X86 seem to get these (constant) allocas merged into the stack frame. This is true also without this patch, but I am not sure why. In this case it seems even more preferred to avoid the dynamic_stackalloca nodes whenever possible...

That's strange.   Not sure why we should be different from X86 here.  Can you investigate?

> With dynamic allocas, it seems wise to always probe no matter what the size, but the "tail" in emitProbedAlloca() is not probed. This seems flawed to me:

I agree, we need to always probe for dynamic allocas as far as I can see.  GCC does this as well.

As a general note, the code duplication between inlineStackProbe and emitProbedAlloca is a bit unfortunate, not sure if there's a better way here.

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

https://reviews.llvm.org/D78717