[Openmp-commits] [PATCH] D63009: [OpenMP] Add target task alloc function with device ID

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D63009#1545152 <https://reviews.llvm.org/D63009#1545152>, @gtbercea wrote:

> In D63009#1544984 <https://reviews.llvm.org/D63009#1544984>, @ABataev wrote:
>
> > In D63009#1544900 <https://reviews.llvm.org/D63009#1544900>, @gtbercea wrote:
> >
> > > In D63009#1544758 <https://reviews.llvm.org/D63009#1544758>, @Hahnfeld wrote:
> > >
> > > > Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual `device_id` argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?
> > >
> > >
> > > When I was just passing the default value the LLVM-IR was: i64 -1 i.e. constant, easy to check.
> > >
> > > With the latest change the emitted code is: i64 %123 i.e. where %123 is a local derived from the expression of the device ID.
> >
> >
> > If the value is constant, check for the constant.
>
>
> All the tests here use expressions so the value is never constant. If you'd like me to add a test with constant I can.


Yes, there must be at least one test with the default value.

> 
> 
>> And at least several tests with the expressions should check for the correct value of the expression.
> 
> I'll have to check how to do this. There's nothing that distinguishes an expression that represents the device ID from any other expression in the code.




Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63009/new/

https://reviews.llvm.org/D63009