fhahn marked 4 inline comments as done.
fhahn added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16669
+ExprResult Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall,
+ ExprResult CallResult) {
+ if (checkArgCount(*this, TheCall, 2))
----------------
aaron.ballman wrote:
> fhahn wrote:
> > aaron.ballman wrote:
> > > Do we actually need this parameter?
> > No, it can just return `ExprResult(TheCall)` instead I think!
> Could also return a `bool` and have the caller fiddle with `ExprResult`, too.
> I don't have strong opinions on which way to go though.
updated to return a bool
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16673-16678
+ Expr *A = TheCall->getArg(0);
+ Expr *B = TheCall->getArg(1);
+ QualType TyA = A->getType();
+ QualType TyB = B->getType();
+
+ if (TyA != TyB)
----------------
aaron.ballman wrote:
> fhahn wrote:
> > aaron.ballman wrote:
> > > fhahn wrote:
> > > > aaron.ballman wrote:
> > > > > Should these arguments undergo usual conversions (array to pointer
> > > > > decay, integer promotions, etc)?
> > > > I intentionally went with not performing conversions, because it might
> > > > lead to surprising results. If the arguments have different types, it
> > > > is not clear to me which type should be chosen to try convert the other
> > > > one; e.g. if we have __builtin_elementwise_max(float, int)` should the
> > > > first argument be converted to `int` or the second one to `float`?
> > > >
> > > > Forcing the types to match without conversion seem to make them less
> > > > error-prone to use, but I might be missing some general rule to handle
> > > > type conflicts here?
> > > I'm not certain how these builtins are expected to be used, but if
> > > they're likely to be used with literals, I think we may want integer
> > > promotions because of that alone.
> > >
> > > Btw, given that these only have two arguments, it seems like we could use
> > > the usual arithmetic conversions to pick a common type the same way as
> > > happens for binary operators.
> > >
> > > If you end up not adding any conversions here, we should have a test case
> > > to cover passing in two array objects (which won't decay to pointers).
> > > I'm not certain how these builtins are expected to be used, but if
> > > they're likely to be used with literals, I think we may want integer
> > > promotions because of that alone.
> >
> > Yes, they should ideally work with literals! I think it should be
> > relatively straight-forward to add promotion of literals.
> >
> > > Btw, given that these only have two arguments, it seems like we could use
> > > the usual arithmetic conversions to pick a common type the same way as
> > > happens for binary operators.
> >
> > IIUC this happens in `SemaOverload.cpp`, but I am not sure how builtins
> > would hook into the infrastructure there. Are there other builtins that are
> > similarly overloaded by any chance?
> > Yes, they should ideally work with literals! I think it should be
> > relatively straight-forward to add promotion of literals.
>
> I was thinking of the literal being an `int` and the variable being a
> `short`, so the literal isn't what needs promotion in that case. That said, I
> suppose character literals in C++ would promote from `char` to `int`, so
> there are some literals that could promote. That'd be a somewhat amusing test
> case for C and C++ both (`__builtin_elementwise_max('a', 1);`).
>
> > IUC this happens in SemaOverload.cpp, but I am not sure how builtins would
> > hook into the infrastructure there. Are there other builtins that are
> > similarly overloaded by any chance?
>
> `Sema::SemaBuiltinUnorderedCompare()` uses this, for example:
> ```
> ExprResult OrigArg0 = TheCall->getArg(0);
> ExprResult OrigArg1 = TheCall->getArg(1);
>
> // Do standard promotions between the two arguments, returning their common
> // type.
> QualType Res = UsualArithmeticConversions(
> OrigArg0, OrigArg1, TheCall->getExprLoc(), ACK_Comparison);
> if (OrigArg0.isInvalid() || OrigArg1.isInvalid())
> return true;
> ```
> hat'd be a somewhat amusing test case for C and C++ both
> (__builtin_elementwise_max('a', 1);).
I added ` i1 = __builtin_elementwise_max(1, 'a');` to the codegen tests.
> Sema::SemaBuiltinUnorderedCompare() uses this, for example:
Thanks a lot! I updated to code to use `UsualArithmeticConversions` and added
the tests you suggested (hope I didn't miss any important ones). Looks like all
should work as expected now.
================
Comment at: clang/test/Sema/builtins-elementwise-math.c:42
+ // expected-error@-1 {{argument types do not match, 'float4' (vector of 4
'float' values) != 'int3' (vector of 3 'int' values)}}
+}
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > fhahn wrote:
> > > aaron.ballman wrote:
> > > > Additional tests I would like to see:
> > > > ```
> > > > int i;
> > > > short s;
> > > >
> > > > __builtin_elementwise_min(i, s); // ok (integer promotions)?
> > > >
> > > > enum e { one, two };
> > > > __builtin_elementwise_min(one, two); // ok (using enums)?
> > > >
> > > > enum f { three };
> > > > __builtin_elementwise_min(one, three); // ok (different enums)?
> > > >
> > > > _ExtInt(32) ext;
> > > > __builtin_elementwise_min(ext, ext); // ok (using bit-precise integers)?
> > > >
> > > > const int ci;
> > > > __builtin_elementwise_min(i, ci); // ok (qualifiers don't match)?
> > > > ```
> > > Thanks I'll add those!
> > >
> > > at the moment `__builtin_elementwise_min(i, s); // ok (integer
> > > promotions)?` would be rejected, as per my response in Sema.
> > >
> > > The other currently are not handled properly, which I'll fix!
> > I'd still like to see the test case added so it's clear we intend to reject
> > it. It may also be wise to add a test case involving an integer literal and
> > a `short` variable cast to `int` to make it clear that you have to add
> > casts sometimes to make this work.
> >
> > Another interesting test is where sugars don't match. e.g.,
> > ```
> > int i;
> > __attribute__((address_space(0))) int addr_space_i;
> > typedef int foo;
> >
> > __builtin_elementwise_min(i, addr_space_i); // ok (attributes don't match)?
> > __builtin_elementwise_min(i, foo); // ok (sugar doesn't match)?
> > ```
> You might also need to handle something like:
> ```
> int (i);
> int j;
> __builtin_elementwise_min(i, j); // May get a type mismatch here
> ```
> So you may need to do some type canonicalization to avoid these sorts of
> issues.
Thanks, I updated the code to check the canonical type.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111985/new/
https://reviews.llvm.org/D111985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
