This is an archive of the discontinued LLVM Phabricator instance.

[GISel] Check useLoadStackGuardNode() before generating LOAD_STACK_GUARD
ClosedPublic

Authored by Kai on Jul 11 2022, 11:42 AM.

Details

Summary

When lowering llvm::stackprotect intrinsic, the SDAG implementation
checks function useLoadStackGuardNode() to either create a
LOAD_STACK_GUARD node or use the first argument of the intrinsic.
This check is not present in the IRTranslator, which results in always
generating a LOAD_STACK_GUARD even if the target does not support it.

Diff Detail

Event Timeline

Kai created this revision.Jul 11 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 11:42 AM
Kai requested review of this revision.Jul 11 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 11:42 AM
arsenm added inline comments.Jul 11 2022, 12:17 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2083

Don't see why getStackGuard doesn't just return the virtual register it creates itself

llvm/test/CodeGen/PowerPC/GlobalISel/ppc-irtranslator-stackprotect.ll
3

Is this actually setting the triple correctly? The OS should be in the 3rd position?

Kai updated this revision to Diff 443735.Jul 11 2022, 1:21 PM

Fixed triple.

Kai marked an inline comment as done.Jul 11 2022, 1:23 PM
Kai added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2083

Sorry, do you mean that getStackGuard() should create the virtual register? Currently it uses the register passed as argument.
I guess that should work as LLVM always uses a pointer as type for the stack guard value.

arsenm added inline comments.Jul 11 2022, 1:26 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2083

Yes. These APIs that take output registers are a bit awkward

Kai added inline comments.Jul 11 2022, 2:05 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2083

That is not as trivial as it seems.
In emitSPDescriptorParent(), the type of the guard value is deliberately changed to a scalar similar in size of a pointer, to match the type of the G_LOAD instruction loading the guard value from the stack slot.
That is very nicely demonstrated in test case AArch64/GlobalISel/irtranslator-delayed-stack-protector.ll.
I can make this change but I would prefer to do it in a separate change, as it is really a different goal ("unifying use of getStackGuard()").

arsenm accepted this revision.Jul 11 2022, 2:57 PM
This revision is now accepted and ready to land.Jul 11 2022, 2:57 PM
This revision was landed with ongoing or failed builds.Jul 12 2022, 8:45 AM
This revision was automatically updated to reflect the committed changes.