[Beignet] [PATCH] GBE/libocl: Add beignet_get_time_stamp() to get timestamp.

[Zhigang Gong]

Hi Ruiling,

I have some comments regards to this patch as below:

The first comment is about the naming rule of the beignet intel specific
extension. So far we have implemented two extensions, they are as
below:

short __gen_ocl_simd_any(short);
short __gen_ocl_simd_all(short);

Now we have struct time_stamp beignet_get_time_stamp(void);

> +struct time_stamp {
> +  // time tick
> +  ulong tick;
> +  // If context-switch or frequency change occurs since last read of tm,
> +  // event will be non-zero, otherwise, it will be zero.
> +  uint event;
> +};
> +
> +uint __gen_ocl_read_tm(void);
> +uint __gen_ocl_extract_reg(uint x, int offset);
> +
> +struct time_stamp beignet_get_time_stamp(void);
> +

My suggestion is to make them consistent to each other.
I prefer to use __gen_ocl_get_time_stamp().

The second comment is about the following two internal functions:
> +uint __gen_ocl_read_tm(void);
> +uint __gen_ocl_extract_reg(uint x, int offset);

These two functions should be pure internal function. We should not put
them in the header file. Just put them in the ocl_misc.cl should be good
enough.

The third comment is about the new instructions you introduced
in the IR layer as below:
> +  Instruction READ_ARF(Type type, Register dst, uint32_t regNum, uint32_t subRegNum) {
> +    return internal::ReadARFInstruction(type, dst, regNum, subRegNum).convert();
> +  }

This is a GEN IR level instructions, but the prototype seems too low level for me.
It's better to define some abstract ARF types in this layer, and to determine/choose the actual
regNum/subRegNum in the instruction selection stage according to the actual target platform.

For example, we could define some enum values as
GEN_ARF_TM0
GEN_ARF_XXX

at the ocl_misc.h file, and use this enum value in the ocl kernel.

One scenario is as below:
Although timestamp reg is almost the same among all GENs, some other ARF regs are not.
If some other new extension want to access other ARF and want to use this API in IR layer,
then it will have to check the target platform id at IR level which is not good.

What do you think?

The last comment is for the EXTRACT_REG instruction,
> +  Instruction EXTRACT_REG(Register dst, Register src, uint32_t offset) {
> +    return internal::ExtractRegInstruction(dst, src, offset).convert();
> +  }

I think this could become a more generic extension and may like the below:

int __gen_ocl_region(short offset, int data)

If the offset is a constant, then it does the same thing as your current implementation.
If the offset is a variable, then we could use indirect addressing mode to implement it.
Actually, this is similar to a old extension implemented by Ben originally and be removed
about one year ago due to some reason.