This revision was automatically updated to reflect the committed changes.
Closed by commit rL283264: Separate builtins for x84-64 and i386; implement
__mulh and __umulh (authored by agutowski).
Changed prior to commit:
https://reviews.llvm.org/D24598?vs=73521=73570#toc
Repository:
rL LLVM
rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
lgtm
https://reviews.llvm.org/D24598
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
agutowski added inline comments.
> agutowski wrote in 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**,
agutowski added inline comments.
> rnk wrote in 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
agutowski updated this revision to Diff 73521.
agutowski added a comment.
merge enums
https://reviews.llvm.org/D24598
Files:
include/clang/Basic/BuiltinsX86_64.def
include/clang/Basic/TargetBuiltins.h
lib/Basic/Targets.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Headers/intrin.h
rnk added a comment.
Let's avoid the duplicate enum, otherwise looks good
> TargetBuiltins.h:100
> + /// \brief X86-64 builtins
> + namespace X86_64 {
> + enum {
I think this would be better with just one enum to reduce compilation time:
/// \brief X86 builtins
namespace X86 {
agutowski updated this revision to Diff 73375.
agutowski added a comment.
remove BuiltinsX86_32.def; reduce redundancy in x86 builtins info
https://reviews.llvm.org/D24598
Files:
include/clang/Basic/BuiltinsX86_64.def
include/clang/Basic/TargetBuiltins.h
lib/Basic/Targets.cpp
rnk added inline comments.
> agutowski wrote in BuiltinsX86_32.def:1
> 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.
I guess we should remove it. Take a look at isX86_64Builtin in
rnk added inline comments.
> Targets.cpp:2319
> + { #ID, TYPE, ATTRS, HEADER, LANGS, FEATURE },
> +#include "clang/Basic/BuiltinsX86.def"
> +
I'd rather not duplicate this readonly data. I had this clever idea that we'd
do something like:
const Builtin::Info BuiltinInfo[] = {
...
agutowski added inline comments.
> BuiltinsX86_32.def:1
> +//===--- BuiltinsX86_32.def - X86-32 Builtin function database --*- C++
> -*-===//
> +//
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.
agutowski created this revision.
agutowski added reviewers: rnk, thakis, majnemer.
agutowski added a subscriber: cfe-commits.
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
11 matches
Mail list logo
