Page MenuHomePhabricator

[Clang FE, SystemZ] Recognize -mpacked-stack CL option
ClosedPublic

Authored by jonpa on Dec 12 2019, 4:16 PM.

Details

Reviewers
uweigand
bkramer
Summary

Recognize -mpacked-stack from the command line and add a function attribute "mpacked-stack"="true" when passed.

I compared building benchmarks with -mpacked-stack vs master with this change in SystemZFrameLowering.cpp:

static bool usePackedStack(MachineFunction &MF) {
 -  bool HasPackedStackAttr = MF.getFunction().hasFnAttribute("packed-stack");
 +  bool HasPackedStackAttr = true; //MF.getFunction().hasFnAttribute("packed-stack");
    bool IsVarArg = MF.getFunction().isVarArg();

I found that all files look the same, except for all those who contain the __clang_call_terminate function, which gets the default stack layout even when -mpacked-stack is passed. This seems to relate to exceptions, and my guess is this doesn't need to be handled, but it probably could get the "packed-stack" attribute added somewhere in clang/lib/CodeGen if needed...

Would we need to check in the backend that we don't have the attribute "packed-stack"="false", in case somebody would edit that by hand?

Diff Detail

Event Timeline

jonpa created this revision.Dec 12 2019, 4:16 PM

The diag::err_opt_not_valid_on_target message is emitted from CodeGenFunction::StartFunction -- does that mean if you use -mpacked-stack on a non-SystemZ target when compiling a file with many functions, you get this error message many times? If so, this should probably be moved somewhere else. (It looks like the existing check for -mnop-mcount may already have the same problem.

I found that all files look the same, except for all those who contain the __clang_call_terminate function, which gets the default stack layout even when -mpacked-stack is passed. This seems to relate to exceptions, and my guess is this doesn't need to be handled, but it probably could get the "packed-stack" attribute added somewhere in clang/lib/CodeGen if needed...

This function is synthesized in getClangCallTerminateFn and doesn't go through the usual StartFunction. But I guess this doesn't really matter either way.

Would we need to check in the backend that we don't have the attribute "packed-stack"="false", in case somebody would edit that by hand?

Hmm, some places look for an explicit attribute value of "true", e.g. the X86 target here:

bool SoftFloat =
    F.getFnAttribute("use-soft-float").getValueAsString() == "true";

I guess this would be preferable.

The diag::err_opt_not_valid_on_target message is emitted from CodeGenFunction::StartFunction -- does that mean if you use -mpacked-stack on a non-SystemZ target when compiling a file with many functions, you get this error message many times? If so, this should probably be moved somewhere else. (It looks like the existing check for -mnop-mcount may already have the same problem.

No, it seems to only print this one time even in a file with multiple functions.

I found that all files look the same, except for all those who contain the __clang_call_terminate function, which gets the default stack layout even when -mpacked-stack is passed. This seems to relate to exceptions, and my guess is this doesn't need to be handled, but it probably could get the "packed-stack" attribute added somewhere in clang/lib/CodeGen if needed...

This function is synthesized in getClangCallTerminateFn and doesn't go through the usual StartFunction. But I guess this doesn't really matter either way.

ok

Would we need to check in the backend that we don't have the attribute "packed-stack"="false", in case somebody would edit that by hand?

Hmm, some places look for an explicit attribute value of "true", e.g. the X86 target here:

bool SoftFloat =
    F.getFnAttribute("use-soft-float").getValueAsString() == "true";

I guess this would be preferable.

OK, I'll make a separate backend-patch for that.

uweigand accepted this revision.Dec 16 2019, 10:51 AM

OK, then I think this patch LGTM.

This revision is now accepted and ready to land.Dec 16 2019, 10:51 AM
jonpa updated this revision to Diff 234333.Dec 17 2019, 10:00 AM

Removed the "true" value when adding the function attribute, which seems to do what we want.

jonpa requested review of this revision.Dec 17 2019, 10:01 AM
uweigand accepted this revision.Dec 17 2019, 11:13 AM

OK, thanks!

This revision is now accepted and ready to land.Dec 17 2019, 11:13 AM