This is an archive of the discontinued LLVM Phabricator instance.

Separate builtins for x84-64 and i386; implement __mulh and __umulh
ClosedPublic

Authored by agutowski on Sep 14 2016, 7:33 PM.

Details

Summary

We need x86-64-specific builtins if we want to implement some of the MS intrinsics - winnt.h contains definitions of some functions for i386, but not for x86-64 (for example _InterlockedOr64), which means that we cannot treat them as builtins for both i386 and x86-64, because then we have definitions of builtin functions in winnt.h on i386.

Event Timeline

agutowski updated this revision to Diff 71470.Sep 14 2016, 7:33 PM
agutowski retitled this revision from to Separate builtins for x84-64 and i386; implement __mulh and __umulh.
agutowski updated this object.
agutowski added reviewers: rnk, thakis, majnemer.
agutowski added a subscriber: cfe-commits.
agutowski added inline comments.Sep 30 2016, 9:36 AM
include/clang/Basic/BuiltinsX86_32.def
1 ↗(On Diff #71470)

Will we ever need that file? It seems to nicely complement the 64-bit one, but if it's going to be empty forever maybe it's better to remove it.

rnk added inline comments.Oct 3 2016, 4:14 PM
lib/Basic/Targets.cpp
2321

I'd rather not duplicate this readonly data. I had this clever idea that we'd do something like:

const Builtin::Info BuiltinInfo[] = {
...
#include "BuiltinsX86_32.def"
#include "BuiltinsX86.def"
#include "BuiltinsX86_64.def"
};

And then our makeArrayRef call would take the appropriate parts.

rnk added inline comments.Oct 3 2016, 4:20 PM
include/clang/Basic/BuiltinsX86_32.def
1 ↗(On Diff #71470)

I guess we should remove it. Take a look at isX86_64Builtin in Sema/SemaChecking.cpp. It might be good to, as a follow-up change, move those builtins over to BuiltinsX86_64.def.

rnk added a reviewer: hans.Oct 3 2016, 4:21 PM
agutowski updated this revision to Diff 73375.Oct 3 2016, 5:02 PM

remove BuiltinsX86_32.def; reduce redundancy in x86 builtins info

rnk edited edge metadata.Oct 4 2016, 11:09 AM

Let's avoid the duplicate enum, otherwise looks good

include/clang/Basic/TargetBuiltins.h
100

I think this would be better with just one enum to reduce compilation time:

  /// \brief X86 builtins
  namespace X86 {
  enum {
    LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
#include "clang/Basic/BuiltinsX86.def"
    FirstX86_64Builtin,
    LastX86CommonBuiltin = FirstX86_64Builtin - 1,
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
#include "clang/Basic/BuiltinsX86_64.def"
    LastTSBuiltin
  };
  }
agutowski updated this revision to Diff 73521.Oct 4 2016, 11:39 AM
agutowski edited edge metadata.

merge enums

agutowski added inline comments.Oct 4 2016, 11:45 AM
include/clang/Basic/TargetBuiltins.h
100

Nice, thanks!
As far as I see, it creates some inconsistency in usage of the word "last", because it's used wrong everywhere else - LastTSBuiltin is the number of the last target-specific builtin plus one, while LastX86CommonBuiltin doesn't have this additional one. I would leave it like that, but wanted to point that out, in case you think it's not acceptable.

agutowski added inline comments.Oct 4 2016, 11:48 AM
include/clang/Basic/TargetBuiltins.h
100

Ah, now I see that LastTIBuiltin is also what its name suggests, so it's only LastTSBuiltin that's different.

rnk accepted this revision.Oct 4 2016, 2:35 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 4 2016, 2:35 PM
This revision was automatically updated to reflect the committed changes.