sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/test/CodeGen/builtins-ppc-error.c:51
void testCTF(int index) {
- vec_ctf(vsi, index); //expected-error {{argument to
'__builtin_altivec_vcfsx' must be a constant integer}}
- vec_ctf(vui, index); //expected-error {{argument to
'__builtin_altivec_vcfsx' must be a constant integer}}
+ vec_ctf(vsi, index); //expected-error {{argument to
'__builtin_altivec_vcfsx' must be a constant integer}} expected-error
{{argument to '__builtin_altivec_vcfux' must be a constant integer}}
+ vec_ctf(vui, index); //expected-error {{argument to
'__builtin_altivec_vcfsx' must be a constant integer}} expected-error
{{argument to '__builtin_altivec_vcfux' must be a constant integer}}
----------------
hokein wrote:
> sammccall wrote:
> > hmm, this doesn't exactly look right to me - we know the type of `vsi` so
> > we should only be considering the signed case I thought.
> >
> > However the existing diagnostic for `vui` indicates that it's considering
> > the **signed** case, so I guess this is already broken/bad.
> yeah, we should know this is the signed case.
>
> after the macro expansion, the code looks like
>
> ```
> _Generic((vsi), vector int
> : (vector float)__builtin_altivec_vcfsx((vector int)(vsi),
> (index)), vector unsigned int
> : (vector float)__builtin_altivec_vcfux((vector unsigned
> int)(vsi), (index)));
> ```
>
> it is a `GenericSelectionExpr` which contains a switch-like structure (see
> the AST below), so when we traverse it, we will traverse both
> `__builtin_altivec_vcfsx` and `__builtin_altivec_vcfux`.
>
> ```
> `-GenericSelectionExpr 0x9527238 <line:31:3, line:33:88> '__vector float'
> contains-errors
> |-ImplicitCastExpr 0x9527220 <line:31:12, col:16> '__vector int'
> <LValueToRValue>
> | `-ParenExpr 0x9526980 <col:12, col:16> '__vector int' lvalue
> | `-DeclRefExpr 0x9526960 <col:13> '__vector int' lvalue Var 0x9526568
> 'vsi' '__vector int' non_odr_use_unevaluated
> |-VectorType 0x91923c0 '__vector int' altivec 4
> | `-BuiltinType 0x91296e0 'int'
> |-case '__vector int' selected
> | |-VectorType 0x91923c0 '__vector int' altivec 4
> | | `-BuiltinType 0x91296e0 'int'
> | `-CStyleCastExpr 0x9526db8 <line:32:14, col:78> '__vector float'
> contains-errors <Dependent>
> | `-RecoveryExpr 0x9526d70 <col:28, col:78> '<dependent type>'
> contains-errors lvalue
> | |-DeclRefExpr 0x9526bc0 <col:28> '<builtin fn type>' Function
> 0x95269e8 '__builtin_altivec_vcfsx' '__attribute__((__vector_size__(4 *
> sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(int))))
> int, int)'
> | |-CStyleCastExpr 0x9526c68 <col:52, col:68> '__vector int' <BitCast>
> | | `-ImplicitCastExpr 0x9526c50 <col:64, col:68> '__vector int'
> <LValueToRValue> part_of_explicit_cast
> | | `-ParenExpr 0x9526c30 <col:64, col:68> '__vector int' lvalue
> | | `-DeclRefExpr 0x9526be0 <col:65> '__vector int' lvalue Var
> 0x9526568 'vsi' '__vector int'
> | `-ParenExpr 0x9526cb0 <col:71, col:77> 'const int' lvalue
> | `-DeclRefExpr 0x9526c90 <col:72> 'const int' lvalue ParmVar
> 0x95267c8 'index' 'const int'
> `-case '__vector unsigned int'
> |-VectorType 0x9192700 '__vector unsigned int' altivec 4
> | `-BuiltinType 0x9129780 'unsigned int'
> `-CStyleCastExpr 0x95271f8 <line:33:14, col:87> '__vector float'
> contains-errors <Dependent>
> `-RecoveryExpr 0x95271b0 <col:28, col:87> '<dependent type>'
> contains-errors lvalue
> |-DeclRefExpr 0x9527000 <col:28> '<builtin fn type>' Function
> 0x9526e28 '__builtin_altivec_vcfux' '__attribute__((__vector_size__(4 *
> sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(unsigned
> int)))) unsigned int, int)'
> |-CStyleCastExpr 0x95270a8 <col:52, col:77> '__vector unsigned int'
> <BitCast>
> | `-ImplicitCastExpr 0x9527090 <col:73, col:77> '__vector int'
> <LValueToRValue> part_of_explicit_cast
> | `-ParenExpr 0x9527070 <col:73, col:77> '__vector int' lvalue
> | `-DeclRefExpr 0x9527020 <col:74> '__vector int' lvalue Var
> 0x9526568 'vsi' '__vector int'
> `-ParenExpr 0x95270f0 <col:80, col:86> 'const int' lvalue
> `-DeclRefExpr 0x95270d0 <col:81> 'const int' lvalue ParmVar
> 0x95267c8 'index' 'const int'
> ```
>
>
> > However the existing diagnostic for vui indicates that it's considering the
> > signed case, so I guess this is already broken/bad.
>
> hmm, `vsi` and `vui` have the same type `vector signed int`, so I think there
> is a typo for `vui`, it should be `vector unsigned int`?
> it is a GenericSelectionExpr which contains a switch-like structure (see the
> AST below), so when we traverse it, we will traverse both
> __builtin_altivec_vcfsx and __builtin_altivec_vcfux.
OK, that makes sense I guess.
> I think there is a typo for vui, it should be vector unsigned int?
Yep, that definitely looks to be the case.
================
Comment at: clang/test/Sema/__try.c:55
+ // expected-error{{too few arguments to function call, expected 1, have
0}} \
+ // expected-error{{expected ';' after expression}}
}
----------------
hokein wrote:
> sammccall wrote:
> > this seems bad, am I missing something?
> agree, `__except` makes it very confusing intuitively.
>
> what's happening? - while at here, there is no preceding `__try`, clang
> parses `__execept` as a function call to an implicit function, so it expects
> a `;` after ` __except (FilterExpression());`
>
> why don't we hit this before? -- because the `FilterExpression()` is invalid
> (missing an argument), and the whole function call expr is being dropped.
> With recovery-ast, we preserve the whole function call, then we emit the `;`
> diagnostic.
>
> not quite sure on how to make it clearer. I think this is not super critical.
>
>
Yes, this isn't critical and we were just getting a bit lucky before. No need
for changes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89046/new/
https://reviews.llvm.org/D89046
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
