This is an archive of the discontinued LLVM Phabricator instance.

Targets: commonize some stack realignment code
ClosedPublic

Authored by jfb on Jul 13 2015, 2:18 PM.

Details

Summary

This patch does the following:

  • Fix FIXME on needsStackRealignment: it is now shared between multiple targets, implemented in TargetRegisterInfo, and isn't virtual anymore. This will break out-of-tree targets, silently if they used virtual and with a build error if they used override.
  • Factor out canRealignStack as a virtual function on TargetRegisterInfo, by default only looks for the no-realign-stack function attribute.

Multiple targets duplicated the same needsStackRealignment code:

  • Aarch64.
  • ARM.
  • Mips almost: had extra DEBUG diagnostic, which the default implementation now has.
  • PowerPC.
  • WebAssembly.
  • x86 almost: has an extra -force-align-stack option, which the default implementation now has.

The default implementation of needsStackRealignment used to just return false. My current patch changes the behavior by simply using the above shared behavior. This affects:

  • AMDGPU
  • BPF
  • CppBackend
  • MSP430
  • NVPTX
  • Sparc
  • SystemZ
  • XCore
  • Out-of-tree targets

This is a breaking change! make check passes.

The only implementation of the virtual function (besides the slight different in x86) was Hexagon (which did MF.getFrameInfo()->getMaxAlignment() > 8), and potentially some out-of-tree targets. Hexagon now uses the default implementation.

needsStackRealignment was being overwritten in <Target>GenRegisterInfo.inc, to return false as the default also did. That was odd and is now gone.

Diff Detail

Event Timeline

jfb updated this revision to Diff 29606.Jul 13 2015, 2:18 PM
jfb retitled this revision from to Targets: commonize some stack realignment code.
jfb updated this object.
jfb added a reviewer: sunfish.
jfb added a subscriber: llvm-commits.
jfb added a comment.Jul 13 2015, 2:20 PM

I'm open to changing this patch to keep the previous default behavior, it just seemed simpler to put this out as-is for now and see what folks think.

jfb added a comment.Jul 13 2015, 2:27 PM

Should we decide to go with the breaking change, I think we should discuss on the mailing list too.

jfb updated this object.Jul 13 2015, 2:27 PM
jfb updated this object.
jfb updated this object.
sunfish accepted this revision.Jul 13 2015, 2:37 PM
sunfish edited edge metadata.

This looks good to me. It'd be good to get more eyes on this though.

lib/CodeGen/TargetRegisterInfo.cpp
323

I think MIPS' assert makes sense here. If a function "needs" stack realignment, we're going to require that it "can" realign its stack.

This revision is now accepted and ready to land.Jul 13 2015, 2:37 PM
jfb updated this revision to Diff 29713.Jul 14 2015, 2:32 PM
jfb edited edge metadata.
  • Add warning to the default needsStackRealignment implementation.

x86 almost: has an extra force-align-stack option, and is otherwise the same as the default implementation. Should this option be shared by all targets?

Yes, that sounds very useful.

Should we decide to go with the breaking change, I think we should discuss on the mailing list too.

Is this a change for anyone other than Hexagon. Their check seems odd: Are they specifying the right stack alignment. We should find out from them.

mcrosier added a subscriber: mcrosier.

Adding Kryzsztof in case he's like to comment on the Hexagon side.

rnk added a subscriber: rnk.Jul 14 2015, 2:55 PM

This is only a breaking change in the sense that it affects needsStackRealignment() for out of tree targets, right? If not, can you elaborate on what I'm missing?

This looks like a good direction.

lib/CodeGen/TargetRegisterInfo.cpp
323

Did the assertion not make it past the test suite? It looks like you added the dbgs() after this comment.

lib/Target/X86/X86RegisterInfo.cpp
467

This (and other) hasVarSizedObjects() checks should probably be looking for MFI->hasVarSizedObjects() || MFI->hasOpaqueSPAdjustment() as is done above in hasBasePointer. It catches the case where there is some other reason why SP is useless for addressing local variables, like SP-adjusting inline assembly or (ugh) SEH.

jfb updated this revision to Diff 29741.Jul 14 2015, 6:32 PM

This update should address all comments:

  • Add warning to the default needsStackRealignment implementation.
  • Move -force-align-stack out of x86, and make it work for all targets as suggested by hfinkel.
  • Remove x86 override of needsStackRealignment, and make the default one check for -force-align-stack (as the x86 version used to).
  • Remove the Hexagon override of needsStackRealignment: comments on Phabricator seem to agree that the code was wrong, and the default implementation is probably right. All tests still pass.
  • needsStackRealignment is now non-virtual. Out-of-tree targets need to update their code and instead implement canRealignStack. This will be a silent breaking change if code used 'virtual', but will not be silent if they used 'override'.
jfb added a comment.Jul 14 2015, 6:33 PM

I updated the patch to address all comment, except the two below from @rnk.

I'll update the description to accurately describe the current patch.

lib/CodeGen/TargetRegisterInfo.cpp
323

This is a non-fatal debug diagnostic, that'll only appear in a debug build with -debug-only=target-reg-info? It shouldn't hard-fail anything IIUC. Or am I misunderstanding?

lib/Target/X86/X86RegisterInfo.cpp
467

Would you mind if I do it in a separate patch? I'm assuming it'll require a test too :)

jfb updated this object.Jul 14 2015, 6:37 PM

The Hexagon implementation was functionally equivalent to what the new implementation does (minus the attribute checks, etc.). The default stack alignment is 8 and the old function really should have used getStackAlignment, which the new one does. This looks good to me.

jfb added a comment.Jul 17 2015, 4:56 PM

Any further comments / improvements?

rnk accepted this revision.Jul 20 2015, 1:26 PM
rnk added a reviewer: rnk.

lgtm

jfb added inline comments.Jul 20 2015, 1:28 PM
lib/CodeGen/TargetRegisterInfo.cpp
323

I discussed this offline with @rnk, and I'll submit a follow-up to make it a hard-failure (not just debug print). I'm making it a follow-up so it's less likely to get reverted.

This revision was automatically updated to reflect the committed changes.
jfb added inline comments.Jul 20 2015, 5:13 PM
lib/Target/X86/X86RegisterInfo.cpp
467

Follow-up in D11377.

jfb added a comment.Jul 20 2015, 8:39 PM

@rnk / @hfinkel you may be interested in this comment about needsStackRealignment's diagnostic. If you agree I'll submit another patch to remove the diagnostic.

llvm/trunk/lib/CodeGen/TargetRegisterInfo.cpp
327 ↗(On Diff #30206)

I'm not sure this diagnostic makes sense: x86, ARM and MIPS implement canRealignStack, and a few places in the codebase check needsStackRealignment as one of a few conditions which are supposed to be commutable (in a chain of logical ors), or as a condition in a chain of logical ands where the call to canRealignStack was hoisted to be done unconditionally. When this happens, the code assumes that calling needsStackRealignment on invalid functions will just return false and the other checks will handle things. Adding report_fatal_error to canRealignStack breaks that assumption.

I wrote a patch (attached) which tries to fix all of these assumptions, but the code still fails on all the same tests. For example, ARM's hasFP check eventually ends up calling canRealignStack to decide whether getFrameRegister should return FP or SP. The basic situation seems to be sensible (if you can't realign the stack then your frame register is the stack pointer), but making this warning a hard-failure breaks the code.

Similar situations occur for other tests.

I think the right thing to do here is to remove this diagnostic: in some cases it makes sense, but only the (sometimes indirect) caller of needsStackRealignment knows whether that's the case or not. We could instead add an isFatal parameter, but that seems icky because the caller can be indirect (e.g. the previous hasFP example).

Turning this into report_fatal_error makes the following tests fail:

LLVM :: CodeGen/ARM/2010-06-29-PartialRedefFastAlloc.ll
LLVM :: CodeGen/ARM/2011-08-12-vmovqqqq-pseudo.ll
LLVM :: CodeGen/ARM/2012-01-23-PostRA-LICM.ll
LLVM :: CodeGen/ARM/struct_byval.ll
LLVM :: CodeGen/ARM/struct_byval_arm_t1_t2.ll
LLVM :: CodeGen/ARM/thumb1_return_sequence.ll
LLVM :: CodeGen/ARM/vldm-sched-a9.ll
LLVM :: CodeGen/ARM/vst3.ll
LLVM :: CodeGen/Mips/msa/spill.ll
LLVM :: CodeGen/Mips/no-odd-spreg-msa.ll
LLVM :: CodeGen/Thumb/2009-08-20-ISelBug.ll
LLVM :: CodeGen/Thumb/2011-05-11-DAGLegalizer.ll
LLVM :: CodeGen/Thumb/large-stack.ll
LLVM :: CodeGen/Thumb/long.ll
LLVM :: CodeGen/Thumb2/crash.ll
LLVM :: CodeGen/Thumb2/large-call.ll
LLVM :: CodeGen/X86/avx-intel-ocl.ll
LLVM :: CodeGen/X86/avx-load-store.ll
LLVM :: CodeGen/X86/avx-vzeroupper.ll
LLVM :: CodeGen/X86/avx512-intel-ocl.ll
LLVM :: CodeGen/X86/dynamic-allocas-VLAs.ll
LLVM :: CodeGen/X86/half.ll
LLVM :: CodeGen/X86/musttail-fastcall.ll
LLVM :: CodeGen/X86/pr18846.ll
LLVM :: CodeGen/X86/preserve_allcc64.ll
LLVM :: CodeGen/X86/stack-folding-fp-avx1.ll
LLVM :: CodeGen/X86/stack-folding-int-avx1.ll
LLVM :: CodeGen/X86/stack-folding-int-avx2.ll
LLVM :: CodeGen/X86/stack-folding-xop.ll
LLVM :: CodeGen/X86/unaligned-spill-folding.ll
LLVM :: CodeGen/X86/vec_fp_to_int.ll