Page MenuHomePhabricator

[IR] Don't assume all functions are 4 byte aligned

Authored by michaelplatings on Jan 28 2019, 9:11 AM.



This is based on previously discussed patches and

D44781 was abandoned on the understanding that tracking the alignment of function pointers was unlikely to be beneficial.
D55115 was reverted after it emerged that it caused a code size regression because it removed an assumption about the alignment of function pointers.

This patch combines those two:
Add function pointer alignment to DataLayout.
If the function pointer alignment is set, use it. Otherwise, continue to assume that the alignment is 4 as before.

Diff Detail


Event Timeline

Please always upload all patches with -U99999.

Diff with -U99999

efriedma added a subscriber: efriedma.

Missing update to PPC backend.

I'm not convinced that we should be modifying the datalayout here, as opposed to adding some sort of target hook for the optimizer.

It's a little unfortunate that there isn't any way to represent that the alignment of a function pointer is the same as the alignment of the function, but has some known minimum value.

2007 ↗(On Diff #183881)

This needs more description.

1083 ↗(On Diff #183881)

If you fail to find a datalayout, you have to assume the worst case (that the pointer isn't aligned at all).

1087 ↗(On Diff #183881)

Using "4" as a default is dangerous; on x86, the alignment of the pointer corresponds to the alignment of the function, but might be less than 4 if it isn't explicitly specified.

Why do you think this would be better as a target hook, rather than in the datalayout? The datalayout already includes information about alignments of different types, and most of the target hooks are concerned with what IR will be efficient for the target, not things which might affect correctness.

I've thought about it a bit more, and maybe specifying it in the datalayout is appropriate: the representation of function pointers is a fundamental property of the target/ABI. I mean, it's not really that important in most cases... if you conservatively treat all function pointers as completely opaque, you'll usually get reasonable optimization, but it's still a fundamental property of a core IR type.

That said, I think the suggested datalayout modification isn't complete. I think we need two possible specifications:

F<bitwidth>: The alignment of function pointers is independent of the alignment of functions, and is at least <bitwidth>. ARM with Thumb uses F8, PPC use F32/F64 for 32/64-bit targets, MIPS with microMIPS uses F8.
f<bitwidth>: The alignment of function pointers is greater than or equal to the explicit alignment specified on the function, and is at least <bitwidth>. Used for other targets; x86 uses f8, aarch64 uses f32, etc.

There's an argument for using a target hook instead: in some cases, we can tell more about a specific function in some cases, so we want target hook anyway. For example, on ARM, if a function uses an attribute to mark it as an ARM-mode function, the alignment is at least 32 bits; there's no way to specify that in the datalayout. And if we have the target hook, very little code would end up querying the datalayout, so there's hardly any point. (IIRC Chandler actually tried to write some code involving aligned function pointers for LLVM at one point, but it failed because we don't have that target hook.)

Hi @efriedma, sorry for the delayed response.
I've added the features you asked for to DataLayout.
It is intended that this patch preserves the existing behaviour if no function pointer alignment is specified.* Therefore I hope you'll agree that it isn't necessary for me to provide code to use the new feature on all platforms.
I did also look into using a target hook but doing so would require modifying a lot of functions to take a TargetTransformInfo argument. Given that we're in agreement that the DataLayout is an appropriate choice, it seemed like the best option.

  • The exception to this is the case where the Function object doesn't have a parent - no alignment is assumed whereas previously 4-byte alignment was assumed. Hopefully this case is rare enough that code size won't be significantly impacted.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 25 2019, 8:14 AM
michaelplatings marked 4 inline comments as done.Feb 25 2019, 9:34 AM
michaelplatings added inline comments.
1087 ↗(On Diff #183881)

I've put in a comment to say as much. Sadly I'm not in a position to change this behaviour without causing a code size regression.

Chandler, when you have a chance, can you look at the LangRef changes, since you put some thought into the design?

I think the DataLayout/LangRef changes look correct.

I agree it isn't necessary to fix every target in the initial patch; it would take a long time to get review, and no one person can properly test every target. But I'm afraid we'll forget to fix some target if someone doesn't go through and post followup patches for every target. (It doesn't really even matter if the initial patches are right, as long as there's something for the target maintainers to look at.)

7 ↗(On Diff #188184)

Are you just moving this for the unittests? You shouldn't need to; you can just check the result of ConstantExpr::get(Instruction::And, ...

1087 ↗(On Diff #183881)

From the discussion on D55115, adding the right default to x86 (Fn8) is probably enough to avoid the regression, but I guess we can deal with that as a followup.

Please add a comment like // FIXME: This code should be deleted once existing targets have appropriate defaults

miyuki added inline comments.Feb 26 2019, 2:03 AM
651 ↗(On Diff #188184)

This comment needs to be updated

michaelplatings marked 3 inline comments as done.
efriedma accepted this revision.Feb 26 2019, 3:32 PM

Probably worth sending an email to llvmdev noting that the datalayout is changing before you merge this, so a wider audience can review the IR aspects of the change.

Otherwise LGTM

This revision is now accepted and ready to land.Feb 26 2019, 3:32 PM
This revision was automatically updated to reflect the committed changes.

Anyone looking at updating the datalayout for other platforms? (If nobody is looking at all, I'll try to find some time next week, I guess.)