This is an archive of the discontinued LLVM Phabricator instance.

add support functions for hard/soft float multilib distinction
Needs RevisionPublic

Authored by kelledin on Sep 16 2018, 8:41 AM.

Details

Summary

Current clang does not understand about hard-float/soft-float multilib for 32-bit ARM (armhf/armel in Debian parlance). Changes are pending for clang, but they will depend on these functions.

(This is a blocking differential for D52705)

Diff Detail

Event Timeline

kelledin created this revision.Sep 16 2018, 8:41 AM

This looks fine from a quick glance but would you mind quoting the differential revision in Clang (or making one) which is blocked by this change? I currently do not see this linked to any other proposed change.

Ping. Can you provide some broader context for the changes that are blocked on this diff? You said that changes are pending for Clang, could you provide more context, like another differential revision? Without broader context it's hard to justify adding functions to the Support library that aren't being used anywhere and there is no outline or a plan to use them that isn't extremely vague.

This looks fine from a quick glance but would you mind quoting the differential revision in Clang (or making one) which is blocked by this change? I currently do not see this linked to any other proposed change.

See here: https://reviews.llvm.org/D52705

Be aware, though, that the clang changeset is currently not working as intended (I describe the problems in the review summary). I've posted it only for other eyes to review and help figure out what I need to do.

kristina accepted this revision.Oct 1 2018, 12:19 AM

LGTM in that case, though please hold off landing it for 20 min or so, I want to make sure this builds and run minimal regression tests and my buildbot is currently occupied but not for long.

This revision is now accepted and ready to land.Oct 1 2018, 12:19 AM
kristina requested changes to this revision.EditedOct 1 2018, 12:51 AM

Please address this warning, several of your switch statements are missing default cases:

/SourceCache/llvm-trunk-8.0/lib/Support/Triple.cpp:1460:11: warning: 47 enumeration values not handled in switch: 'UnknownArch', 'arm', 'armeb'... [-Wswitch]

Also, minor nitpick, default cases are usually first in switch statements. Aside from that the patch is sound, but the warning needs to be addressed.

This revision now requires changes to proceed.Oct 1 2018, 12:51 AM
peter.smith added a subscriber: peter.smith.

I've added another couple of reviewers.

At this stage I'm not yet sure that it is worth adding this functionality to Triple. In D52705 it seems like this is used to get the HF or non HF string name for the provided triple only when multilibs are used on Arm. As a result a lot of the code in the function (x86 handling) won't ever be exercised as it won't be called for other targets. A name change of the functions to getARMSoftFloatVariant() or getARMHardFloatVariant() and only handle the Arm cases would be a bit more honest, but it also points out that perhaps it would be better to handle as a local function called from findArmEABIMultilibs()?

The similar functions getLittleEndianVariant() and getBigEndianVariant() in Triple are slightly more general. They are called for all targets and there are also at least 3 targets (Arm, AArch64 and Mips) that include the endianness in the Target. This makes it more useful to handle the functionality in Triple.

If the consensus is to go ahead with this in triple please do add some tests to llvm/unittests/ADT/TripleTest.cpp

This is a slight duplication of what we already have in ARMSubtarget.h, and given that this is *only* relevant for AArch32, I see no point in this being in the Triple.

Maybe I'm missing something? Without a use-case, it's hard to understand why you need this at all... Without meaningful tests, it's hard to consider for merging...

rengolin requested changes to this revision.Oct 1 2018, 3:36 AM

Adding more people that work around triples...

I've added another couple of reviewers.

At this stage I'm not yet sure that it is worth adding this functionality to Triple. In D52705 it seems like this is used to get the HF or non HF string name for the provided triple only when multilibs are used on Arm. As a result a lot of the code in the function (x86 handling) won't ever be exercised as it won't be called for other targets. A name change of the functions to getARMSoftFloatVariant() or getARMHardFloatVariant() and only handle the Arm cases would be a bit more honest, but it also points out that perhaps it would be better to handle as a local function called from findArmEABIMultilibs()?

The similar functions getLittleEndianVariant() and getBigEndianVariant() in Triple are slightly more general. They are called for all targets and there are also at least 3 targets (Arm, AArch64 and Mips) that include the endianness in the Target. This makes it more useful to handle the functionality in Triple.

If the consensus is to go ahead with this in triple please do add some tests to llvm/unittests/ADT/TripleTest.cpp

This mostly depends on the Clang change and if it goes through or not, I think. I think it's okay to land this for convenience for now (once the issues are addressed) and if the Clang differential does not get approved revert this, which is not especially difficult since the support lib is quite isolated. The core issue is in whether the diff that depends on this get approved or not, but I think since it does no harm and is not likely to be used by anything else, it would be okay to land for now and revert it if the Clang change is not approved. In either case this is better as a separate diff rather than a part of the Clang-specific diff, because of its isolation.

kristina edited the summary of this revision. (Show Details)Oct 1 2018, 3:54 AM

Maybe I'm missing something? Without a use-case, it's hard to understand why you need this at all... Without meaningful tests, it's hard to consider for merging...

I edited the diff contents to include a reference to the related Clang change since it seems to be getting lost under all the other comments, that's the supposed use case provided by the author.

I think it's okay to land this for convenience for now (once the issues are addressed) and if the Clang differential does not get approved revert this, which is not especially difficult since the support lib is quite isolated.

This is a re-implementation of existing functionality in a completely different part of the code. With time, both implementations can diverge, creating hard-to-spot problems. He had enough of those already between Clang and LLVM (see TargetParser) and LLVM itself (see isLikeA9).

Convenience today usually means disaster tomorrow. :)

The core issue is in whether the diff that depends on this get approved or not, but I think since it does no harm and is not likely to be used by anything else,

The harm it does is code duplication, and not being used by anything else is actually a problem, not a feature.

it would be okay to land for now and revert it if the Clang change is not approved. In either case this is better as a separate diff rather than a part of the Clang-specific diff, because of its isolation.

Committing a patch with the intent to revert is never a good policy.

Also, because we still have repo separation, we *have* to have two separate patches, one for Clang and one for LLVM. But we tend to tie them up in a patch series and commit one after the other, to make sure bisects don't suffer too greatly.

It would be a better strategy to make use of the existing functionality, or to propose a way to either expose that functionality (if not visible from Clang) or to move it to a different place.

Duplicating it would be a bad idea in any case.

kristina added a comment.EditedOct 1 2018, 4:31 AM

Committing a patch with the intent to revert is never a good policy.

There's no intent to revert, there's intent to make it easier for Clang reviewers to not have to apply separate patchsets from LLVMSupport and from the Clang diff. The intent is to allow it to stay until the Clang diff is reviewed and then if it's not accepted, revert the now-unused chunk of code. Anyway, once it's fixed, I'm personally in favor and think this does belong in Triple, but it looks like the consensus is going to be against. That said, it may be related to how I review things since I always apply patches against my LLVM fork and test them before anything, some may not.

There's no intent to revert

Sorry, bad wording. Ignore "intent", I meant "effect", see below.

there's intent to make it easier for Clang reviewers to not have to apply separate patchsets from LLVMSupport and from the Clang diff.

That may be easier for Clang developers, but it creates a serious issue for LLVM developers.

I know we're all supposed to be "the same crowd", but LLVM really is an API, and as such, it must be consistent, cohesive, uncoupled.

The intent is to allow it to stay until the Clang diff is reviewed and then if it's not accepted, revert the now-unused chunk of code.

The intent may be "help clang devs" but the actual *effect* is to add ephemeral duplicated code to LLVM, which is bad.

This has been done somewhat liberally in LLVM, with post-commit review patches, for example in LLD, and has caused a good amount of trouble to other developers that are trying to create more complex (and slower paced) features.

We really shouldn't be adding code that will later be reverted just for the sake of testing other parts of the project.

If enough of us do that on enough parts of the code, it will be impossible to use trunk for anything and we may end up derailing the whole project.

Anyway, once it's fixed, I'm personally in favor and think this does belong in Triple, but it looks like the consensus is going to be against.

If that's the case, then I strongly recommend you make the case with an actual move of the code, showing how much better it will be. :)

We really shouldn't be adding code that will later be reverted just for the sake of testing other parts of the project.

As I said, that's not the intent. The intent is to allow a blocking change through that, IMHO, is extremely unlikely to start being used by other LLVM projects left and right. But anyway, since you've been a reviewer for far longer than I have and have more experience with situations like these, I'll defer to your judgement.

kristina resigned from this revision.Oct 23 2018, 8:49 PM