This is an archive of the discontinued LLVM Phabricator instance.

Add errors for tiny codemodel on targets other than AArch64
ClosedPublic

Authored by dmgreen on Aug 1 2018, 6:40 AM.

Details

Summary

Adds fatal errors for any target that does not support the Tiny codemodel.

Not 100% sure what the best way to do this is. Commoning the getEffectiveCodeModel function into single place seems like a sensible idea, but where that function goes I'm not sure of.

Split out of D49673

Diff Detail

Repository
rL LLVM

Event Timeline

dmgreen created this revision.Aug 1 2018, 6:40 AM

I don't have any better suggestions about where to move the function right now. I have a couple of minor suggestions.

include/llvm/Target/TargetMachine.h
359 ↗(On Diff #158524)

I have a mild personal preference not to use a default parameter here, in other words pass in default code model for all callers and not just Lanai and WebAssembly. CodeModel::Small is by far the most common but I think it makes it easier to see what the default is from the individual backend without having to go up a few levels.

Not a strong opinion, so feel free to ignore.

361 ↗(On Diff #158524)

Is it worth checking that the Kernel code model is not supported as well? As far as I can tell this is only supported on AArch64. Admittedly it looks like the chances of someone using that by mistake is much lower than CodeModel::Tiny

dmgreen added inline comments.Aug 6 2018, 2:57 AM
include/llvm/Target/TargetMachine.h
359 ↗(On Diff #158524)

Yeah, I agree. That sounds sensible.

361 ↗(On Diff #158524)

Yep. As far as I can tell it may do something on X86 and AArch64. The others it is ignored on or treated the same as the Small model.

dmgreen updated this revision to Diff 159264.Aug 6 2018, 2:57 AM

Review updates.

Ping. Everyone happy with this now the main part has gone in?

Ping. Everyone happy with this now the main part has gone in?

I'm happy from the Arm/AArch64 side.

asb added a comment.Nov 12 2018, 7:08 AM

This seems a positive improvement and I don't want to bikeshed it too much, but it does feel a bit arbitrary to reject tiny and kernel codemodels in getEffectiveCodeModel. You could imagine having a virtual isSupportedCodeModel. Might be more hassle than it's worth though...

include/llvm/Target/TargetMachine.h
355–357 ↗(On Diff #159264)

What about adding something explicit such as: "Targets which support the tiny/kernel code models or require more complex code model selection logic should implement and call their own variant of this function."?

Makes it clear to anyone looking at it that there's not an extension point they're missing - the intended approach is just to go and implement your own version.

dmgreen updated this revision to Diff 173869.Nov 13 2018, 10:04 AM
dmgreen marked an inline comment as done.

I'm happy to change stuff around. I believe it's the way it is at the moment because of the place this is called, half way into a constructor (making virtuals difficult) and some archs requiring different pieces of info (like aarch64 being different for JIT's). I figured if in doubt, don't change too much at once.

This is a rebase, plus your suggested comment. Let me know if this is OK as-is, or I should change it up more.

asb accepted this revision.Nov 29 2018, 2:02 AM

Looks good to me.

This revision is now accepted and ready to land.Nov 29 2018, 2:02 AM
This revision was automatically updated to reflect the committed changes.