[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
labrinea abandoned this revision. labrinea added a comment. Hi Renato, apologies for the long silence. Unfortunately this work is more complicated than I initially thought. We'll have to rethink about it thoroughly. I am going to abandon the patch for now. Thank you for reviewing this. https://reviews.llvm.org/D26968 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
labrinea added a comment. Your suggestion that if all four options are mutually exclusive, then they should be a single integer option, totally makes sense to me. We could use a single integer option and make the old boolean flags deprecated (i.e. map '-fshort-enums' and 'fno-short enums' to the new integer option). My concern is that we have to be GCC compatible. I will communicate this to the GNU community. https://reviews.llvm.org/D26968 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
rengolin added a comment. How does GCC behave with those arguments? Thinking back now, we may be required to follow, as the principle of least surprise has to hold. https://reviews.llvm.org/D26968 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
rengolin added inline comments.
Comment at: lib/Basic/Targets.cpp:5407
+ Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+ Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
labrinea wrote:
> rengolin wrote:
> > Isn't ABIEnums 4? Shouldn't this be:
> >
> > Opts.ShortEnums || !Opts.ABIEnums
> >
> > Is it even valid to have ABIEnums && ShortEnums at the same time?
> My understanding is that ABIEnums requires 32-bit enums across an
> ABI-complying interface, but allows short enums outside of it. Therefore
> __ARM_SIZEOF_MINIMAL_ENUM could be any of 1 or 4.
>
> ABIEnums && ShortEnums cannot be both set at the same time.
If all four options are mutually exclusive, than they should be a single
integer option, not multiple boolean ones.
Comment at: lib/Driver/Tools.cpp:5731
+
+ // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+ Arg *Enum;
labrinea wrote:
> rengolin wrote:
> > If this is true, why go the extra complexity of mapping all possible
> > states? Why not just use one line:
> >
> > Enum = Args.getLastArg(options::OPT_fno_enums,
> > options::OPT_fshort_enums, options::OPT_fabi_enums);
> >
> > and be done with it? Short and ABI are not compatible, it's either one or
> > the other.
> I don't like this complicated logic either but it can handle cases like:
> -fshort-enums -fno-enums -fabi-enums -fno-abi-enums
> where -fno-enums should win. I've added a test for this sequence
> (test/Driver/clang_f_opts.c, line 459)
> The suggested logic would ignore '-fno-abi-enums', which is the last
> argument. If we are happy with rejecting all the preceding flags and keeping
> just the last one this would work:
> ```
> Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums,
> options::OPT_fno_short_enums,
> options::OPT_fabi_enums, options::OPT_fno_abi_enums);
> ```
> Alternatively an equally ugly logic that handles complicated sequences is:
> ```
> if (Arg *Enum = Args.getLastArg(options::OPT_fno_enums,
> ShortEnums ? options::OPT_fshort_enums : options::OPT_INVALID,
>ABIEnums ? options::OPT_fabi_enums : options::OPT_INVALID)) {
> ```
I disagree. These are different flags controlling the same thing, so the
last-option wins.
How does -ffast-math vs its internal options are handled? How is -O handled? Or
the other ABI flags like vfp, hard-float?
Easier still, have a look at unsigned/signed char below. Last arg wins.
https://reviews.llvm.org/D26968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
labrinea added inline comments.
Comment at: lib/Basic/Targets.cpp:5407
+ Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+ Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
rengolin wrote:
> Isn't ABIEnums 4? Shouldn't this be:
>
> Opts.ShortEnums || !Opts.ABIEnums
>
> Is it even valid to have ABIEnums && ShortEnums at the same time?
My understanding is that ABIEnums requires 32-bit enums across an ABI-complying
interface, but allows short enums outside of it. Therefore
__ARM_SIZEOF_MINIMAL_ENUM could be any of 1 or 4.
ABIEnums && ShortEnums cannot be both set at the same time.
Comment at: lib/Driver/Tools.cpp:5731
+
+ // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+ Arg *Enum;
rengolin wrote:
> If this is true, why go the extra complexity of mapping all possible states?
> Why not just use one line:
>
> Enum = Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums,
> options::OPT_fabi_enums);
>
> and be done with it? Short and ABI are not compatible, it's either one or the
> other.
I don't like this complicated logic either but it can handle cases like:
-fshort-enums -fno-enums -fabi-enums -fno-abi-enums
where -fno-enums should win. I've added a test for this sequence
(test/Driver/clang_f_opts.c, line 459)
The suggested logic would ignore '-fno-abi-enums', which is the last argument.
If we are happy with rejecting all the preceding flags and keeping just the
last one this would work:
```
Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums,
options::OPT_fno_short_enums,
options::OPT_fabi_enums, options::OPT_fno_abi_enums);
```
Alternatively an equally ugly logic that handles complicated sequences is:
```
if (Arg *Enum = Args.getLastArg(options::OPT_fno_enums,
ShortEnums ? options::OPT_fshort_enums : options::OPT_INVALID,
ABIEnums ? options::OPT_fabi_enums : options::OPT_INVALID)) {
```
https://reviews.llvm.org/D26968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26968: [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
rengolin added a comment.
Hi Alexandros,
My interpretation of `Tag_ABI_enum_size` is that value 3 is that **all** enums
are 32-bit and value 4 is that only those ABI-visible (ie. public interfaces)
have to be 32-bits. This is similar to the other PCS tags.
So, the table is:
- 0: no enums
- 1: short enums
- 2: all 32-bit
- 3: public 32-bit
With the default being 2 in GCC.
My reading of your code here is that you assume short|abi = short, which
doesn't match the table in the ABI.
Comment at: lib/Basic/Targets.cpp:5407
+ Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
+ Opts.ShortEnums || Opts.ABIEnums ? "1" : "4");
Isn't ABIEnums 4? Shouldn't this be:
Opts.ShortEnums || !Opts.ABIEnums
Is it even valid to have ABIEnums && ShortEnums at the same time?
Comment at: lib/Driver/Tools.cpp:5731
+
+ // The last of -fno-enums, -fshort-enums, -fabi-enums wins.
+ Arg *Enum;
If this is true, why go the extra complexity of mapping all possible states?
Why not just use one line:
Enum = Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums,
options::OPT_fabi_enums);
and be done with it? Short and ABI are not compatible, it's either one or the
other.
Comment at: lib/Driver/Tools.cpp:5741
+ else
+Enum = Args.getLastArg(options::OPT_fno_enums);
+ if (Enum) {
This code is confusing and merging with each other. Please add some empty lines
between them.
https://reviews.llvm.org/D26968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
