Page MenuHomePhabricator

[X86] Add NumRegisterParameters Module Flag

Authored by niravd on Nov 23 2016, 8:20 AM.

Diff Detail


Event Timeline

niravd updated this revision to Diff 79089.Nov 23 2016, 8:20 AM
niravd retitled this revision from to Propagate NumRegisterParameters to Backend.
niravd added reviewers: rnk, mkuper.
niravd added a subscriber: llvm-commits.
mkuper edited edge metadata.Nov 23 2016, 9:39 AM

So, I remember back when I implemented the MCU ABI rnk and I discussed this, and didn't really reach a definitive conclusion.

This would make -mregparm behavior match GCC, but would be an ABI break for clang.
You may want to surface this on llvm-dev, and check that doesn't come as a surprise to anyone.

Another issue is that we need to serialize this as part of the IR for the function.
Since we don't use inreg attributes, it needs to either be a part of the calling convention (although we currently have no way to parametrize the CC, I think), or a function attribute.

rnk edited edge metadata.Nov 23 2016, 9:49 AM

This seems like a good use case for module flags, actually. Check out Module::ModFlagBehavior::Error in particular.

I'm not sure. It basically comes down to - do we want to ever be able to combine (say, link) -mregparm code with non-mregparm code.

For the MCU this was a non-issue, because the answer is always no.

niravd updated this revision to Diff 79946.Dec 1 2016, 10:35 AM
niravd edited edge metadata.

Change propagate to use subtarget features

niravd retitled this revision from Propagate NumRegisterParameters to Backend to [X86] Add Regparm Subtarget Attributes.Dec 2 2016, 6:46 AM
niravd updated this revision to Diff 85928.Jan 26 2017, 9:06 AM

Change to llvm module flag

jlebar added a subscriber: jlebar.Jan 26 2017, 10:12 AM
jlebar added inline comments.
21 ↗(On Diff #85928)

Hm, this is a bummer, but I don't immediately see a better way to do it.

niravd retitled this revision from [X86] Add Regparm Subtarget Attributes to [X86] Add NumRegisterParameters Module Flag.Jan 30 2017, 7:05 AM
rengolin removed a subscriber: rengolin.Jan 30 2017, 7:49 AM
rnk added inline comments.Jan 30 2017, 5:00 PM
421 ↗(On Diff #85928)

I'd like this to be conditional on NumRegisterParameters being non-zero, so that it doesn't pollute the vast majority of modules that don't use -mregparm. I think you will get the right LTO diagnostic behavior if you use llvm::Module::Require instead of Error here.

21 ↗(On Diff #85928)

I think this can be removed if the module flag is made conditional.

niravd added inline comments.Feb 6 2017, 12:52 PM
421 ↗(On Diff #85928)

My understanding is that Require only checks the post-linking values, so we'd not error when linking regparm 0 with regparm N.

That said, it'd be easy enough to restrict emitting the module flag to target that actually depend on it, i.e. X86-32. That would get us 90% of the way.

rnk accepted this revision.Feb 13 2017, 5:14 PM


421 ↗(On Diff #85928)

Hm, nevermind then. Let's go with what you have.

5 ↗(On Diff #85928)

Probably worth checking that this calls llvm.memcpy without any inreg attributes.

10 ↗(On Diff #85928)

Probably worth CHECKs for whatever we do for this today.

This revision is now accepted and ready to land.Feb 13 2017, 5:14 PM
This revision was automatically updated to reflect the committed changes.

Most other module flags are added at CodeGenModule::Release(). For consistency, could this code be in CodeGenModule::Release() as well?

No intention of any major work intended... just moved the code (locally) to Release and regression tests were still passing.

I must be missing something - isn't the module flag is used in X86ISelLowering which is called after CodeGenModule::Release() is done?