Preserve vec3 type.

[jaykang10]

Summary

Preserve vec3 type with CodeGen option.

[jaykang10]

Added -f prefix to option name.

[jaykang10]

Fixed typo.

[Anastasia]

Could you please add your test here (probably goes to test/CodeGenOpenCL)?

include/clang/Driver/CC1Options.td

661 ↗

(On Diff #91307)

Can we drop "operations"?

[jaykang10]

Changed help text for option and Added test file.

[jaykang10]

In D30810#697760, @Anastasia wrote:

Could you please add your test here (probably goes to test/CodeGenOpenCL)?

Oops!
I am so sorry. I missed it. I have updated it.

[ahatanak]

Could you elaborate on the motivation for this change?

I was wondering why clang (CodeGen) needed the help of a command line option to decide whether vec3 should be converted to vec4. Can it just preserve vec3 when the architecture is spir?

[jaykang10]

In D30810#699006, @ahatanak wrote:

Could you elaborate on the motivation for this change?

I was wondering why clang (CodeGen) needed the help of a command line option to decide whether vec3 should be converted to vec4. Can it just preserve vec3 when the architecture is spir?

Ah, sorry... I forgot to link the discussion cfe-dev. http://lists.llvm.org/pipermail/cfe-dev/2017-March/052956.html I hope it is helpful for you.

[Anastasia]

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

[jaykang10]

In D30810#699428, @Anastasia wrote:

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

Probably, No... the load/store with vec3 generates vec4 temporarily to be aligned but builtin_astype needs to generate vec4 from vec3 or vice versa because spec allows it on 6.2.4.2. As we discussed on previous e-mails, we would need to add intrinsic function for builtin_astype on llvm and add default behavior on llvm's codegen. It is beyond clang and I am not sure llvm's IR and codegen people allow it... If you have another idea or I missed something, please let me know.

[bruno]

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

My colleague and I are implementing a transformation pass between LLVM IR and another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 -> vec4 transformation? Adding a flag here just for that seems very odd.

[jaykang10]

In D30810#699695, @bruno wrote:

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

My colleague and I are implementing a transformation pass between LLVM IR and another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 -> vec4 transformation? Adding a flag here just for that seems very odd.

Hi Bruno,

Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered why clang generates the vec4 for vec3 load/store. As you can see the comment on clang's code, they are generated for better performance. I had a questions at this point. clang should consider vector load/store aligned by 4 regardless of target??? llvm's codegen could handle vec3 according to targets' vector load/store with their alignment. I agree the flag looks odd but I was concerned some llvm's targets depend on the vec4 so I suggested to add the flag. If I missed something, please let me know.

[Anastasia]

In D30810#699827, @jaykang10 wrote:

In D30810#699695, @bruno wrote:

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

My colleague and I are implementing a transformation pass between LLVM IR and another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 -> vec4 transformation? Adding a flag here just for that seems very odd.

Hi Bruno,

Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered why clang generates the vec4 for vec3 load/store. As you can see the comment on clang's code, they are generated for better performance. I had a questions at this point. clang should consider vector load/store aligned by 4 regardless of target??? llvm's codegen could handle vec3 according to targets' vector load/store with their alignment. I agree the flag looks odd but I was concerned some llvm's targets depend on the vec4 so I suggested to add the flag. If I missed something, please let me know.

I think doing this transformation in a frontend was not the best choice because we are losing the source information too early. But some targets can offer a better support for vec3 natively than converting to vec4. Regarding the option, I am wondering whether adding this as a part of TargetInfo would be a better solution. Although, I don't think any currently available upstream targets support this.

[jaykang10]

In D30810#700476, @Anastasia wrote:

In D30810#699827, @jaykang10 wrote:

In D30810#699695, @bruno wrote:

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

My colleague and I are implementing a transformation pass between LLVM IR and another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 -> vec4 transformation? Adding a flag here just for that seems very odd.

Hi Bruno,

Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered why clang generates the vec4 for vec3 load/store. As you can see the comment on clang's code, they are generated for better performance. I had a questions at this point. clang should consider vector load/store aligned by 4 regardless of target??? llvm's codegen could handle vec3 according to targets' vector load/store with their alignment. I agree the flag looks odd but I was concerned some llvm's targets depend on the vec4 so I suggested to add the flag. If I missed something, please let me know.

I think doing this transformation in a frontend was not the best choice because we are losing the source information too early. But some targets can offer a better support for vec3 natively than converting to vec4. Regarding the option, I am wondering whether adding this as a part of TargetInfo would be a better solution. Although, I don't think any currently available upstream targets support this.

In D30810#700476, @Anastasia wrote:

In D30810#699827, @jaykang10 wrote:

In D30810#699695, @bruno wrote:

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

My colleague and I are implementing a transformation pass between LLVM IR and another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 -> vec4 transformation? Adding a flag here just for that seems very odd.

Hi Bruno,

Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered why clang generates the vec4 for vec3 load/store. As you can see the comment on clang's code, they are generated for better performance. I had a questions at this point. clang should consider vector load/store aligned by 4 regardless of target??? llvm's codegen could handle vec3 according to targets' vector load/store with their alignment. I agree the flag looks odd but I was concerned some llvm's targets depend on the vec4 so I suggested to add the flag. If I missed something, please let me know.

I think doing this transformation in a frontend was not the best choice because we are losing the source information too early. But some targets can offer a better support for vec3 natively than converting to vec4. Regarding the option, I am wondering whether adding this as a part of TargetInfo would be a better solution. Although, I don't think any currently available upstream targets support this.

I have a fundamental question for vec3 load/store again. I have checked how llvm's codegen handles the vec3 and vec4 load/store. Assuming that target's action is widen for vector type unsupported. If they have same alignment, the llvm's codegen generates one vector load for both of them. But it handles store differently between vec3 and vec4 on DAGTypeLegalizer because vec4 store writes place over vec3 type. For example,

source code

typedef float float3 __attribute__((ext_vector_type(3)));

void foo(float3 *a, float3 *b) {
  *a = *b; 
}

LLVM IR for vec4 (it is original clang's output)

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %castToVec4 = bitcast <3 x float>* %b to <4 x float>*
  %loadVec4 = load <4 x float>, <4 x float>* %castToVec4, align 16
  %storetmp = bitcast <3 x float>* %a to <4 x float>*
  store <4 x float> %loadVec4, <4 x float>* %storetmp, align 16
  ret void
}

LLVM IR for vec3

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %0 = load <3 x float>, <3 x float>* %b, align 16
  store <3 x float> %0, <3 x float>* %a, align 16
  ret void
}

As you can see above IR for vec4, "store <4 x float> %loadVec4, <4 x float>* %storetmp, align 16" writes place beyond float3. Is the behavior correct??? I can not find which spec or document mentions it... The vec3 store are lowered to more instructions such as stores and extractelements but the behavior is correct. If the vec3 --> vec4 store is not correct, we could add vec3 as default in TargetInfo. Additionally, if the vec3 --> vec4 store is correct, we could modify llvm codegen's widen vector store for vec3 to generate one vector store like vec4. If I missed something, please let me know.

[ahatanak]

It looks like converting vec3 to vec4 is incorrect in some cases. In the following program, IRGen emits "store <4 x float>" to store g1 to *a, which will overwrite s1.f2.

typedef __attribute__((__ext_vector_type__(3))) float float3;

struct S1 {
  float3 f1;
  float f2;
};

float3 g1;

void __attribute__((noinline)) foo2(float3 *a) {
  *a = g1;
}

void foo1() {
  struct S1 s1;
  foo2(&s1.f1);
}

[ahatanak]

Actually, it's not a mis-compile. The record layout shows that there is a padding before field f2 and f2 starts at byte 16. So using "store <4 x float>" doesn't overwrite the field.

[jaykang10]

In D30810#701132, @ahatanak wrote:

Actually, it's not a mis-compile. The record layout shows that there is a padding before field f2 and f2 starts at byte 16. So using "store <4 x float>" doesn't overwrite the field.

It depends on float3's alignment. I guess the float3's alignment is 16 on your target so there is a padding bytes before f2 to be aligned by 16. If you add attribute((packed)) on struct type, you could see overwrite.

[bruno]

Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered why clang generates the vec4 for vec3 load/store. As you can see the comment on clang's code, they are generated for better performance. I had a questions at this point. clang should consider vector load/store aligned by 4 regardless of target???

I believe the assumption is more practical: most part of upstream llvm targets only support vectors with even sized number of lanes. And in those cases you would have to expand to a 4x vector and leave the 4th element as undef anyway, so it was done in the front-end to get rid of it right away. Probably GPU targets do some special tricks here during legalization.

llvm's codegen could handle vec3 according to targets' vector load/store with their alignment. I agree the flag looks odd but I was concerned some llvm's targets depend on the vec4 so I suggested to add the flag. If I missed something, please let me know.

My guess here is that targets that do care are looking through the vector shuffle and customizing to whatever seems the best to them. If you wanna change the default behavior you need some data to show that your model solves a real issue and actually brings benefits; do you have any real examples on where that happens, or why GPU targets haven't yet tried to change this? Maybe other custom front-ends based on clang do? Finding the historical reason (if any) should be a good start point.

[jaykang10]

I believe the assumption is more practical: most part of upstream llvm targets only support vectors with even sized number of lanes. And in those cases you would have to expand to a 4x vector and leave the 4th element as undef anyway, so it was done in the front-end to get rid of it right away. Probably GPU targets do some special tricks here during legalization.

I have compiled below code, which current clang generates for vec3, using llc with amdgcn target.

LLVM IR (vec3 --> vec4)

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %castToVec4 = bitcast <3 x float>* %b to <4 x float>*
  %loadVec4 = load <4 x float>, <4 x float>* %castToVec4, align 16
  %storetmp = bitcast <3 x float>* %a to <4 x float>*
  store <4 x float> %loadVec4, <4 x float>* %storetmp, align 16
  ret void
}

SelectionDAG after legalization.

Legalized selection DAG: BB#0 'foo:entry'
SelectionDAG has 43 nodes:
  t0: ch = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
    t9: i64 = add t2, Constant:i64<40>
  t10: i32,ch = load<LD4[undef(addrspace=2)](nontemporal)(dereferenceable)(invariant)> t0, t9, undef:i64
    t4: i64 = add t2, Constant:i64<36>
  t6: i32,ch = load<LD4[undef(addrspace=2)](nontemporal)(dereferenceable)(invariant)> t0, t4, undef:i64
  t12: ch = TokenFactor t6:1, t10:1
      t32: v4i32 = BUILD_VECTOR t22, t25, t27, t29
    t21: v4f32 = bitcast t32
  t18: v4i32 = bitcast t21
  t22: i32,ch = load<LD4[%castToVec4](align=16)> t12, t10, undef:i32
  t24: i32 = add t10, Constant:i32<4>
  t25: i32,ch = load<LD4[%castToVec4+4]> t12, t24, undef:i32
  t26: i32 = add t24, Constant:i32<4>
  t27: i32,ch = load<LD4[%castToVec4+8](align=8)> t12, t26, undef:i32
    t28: i32 = add t26, Constant:i32<4>
  t29: i32,ch = load<LD4[%castToVec4+12]> t12, t28, undef:i32
  t31: ch = TokenFactor t22:1, t25:1, t27:1, t29:1
        t35: i32 = extract_vector_elt t18, Constant:i32<0>
      t36: ch = store<ST4[%storetmp](align=16)> t31, t35, t6, undef:i32
        t38: i32 = extract_vector_elt t18, Constant:i32<1>
        t39: i32 = add t6, Constant:i32<4>
      t40: ch = store<ST4[%storetmp+4]> t31, t38, t39, undef:i32
        t42: i32 = extract_vector_elt t18, Constant:i32<2>
        t44: i32 = add t6, Constant:i32<8>
      t45: ch = store<ST4[%storetmp+8](align=8)> t31, t42, t44, undef:i32
        t47: i32 = extract_vector_elt t18, Constant:i32<3>
        t49: i32 = add t6, Constant:i32<12>
      t50: ch = store<ST4[%storetmp+12]> t31, t47, t49, undef:i32
    t51: ch = TokenFactor t36, t40, t45, t50
  t17: ch = ENDPGM t51

As you can see, the SelectionDAG still has 4 load/store.

Assembly output

	.section	.AMDGPU.config
	.long	47176
	.long	11272257
	.long	47180
	.long	132
	.long	47200
	.long	0
	.long	4
	.long	0
	.long	8
	.long	0
	.text
	.globl	foo
	.p2align	8
	.type	foo,@function
foo:                                    ; @foo
; BB#0:                                 ; %entry
	s_load_dword s2, s[0:1], 0x9
	s_load_dword s0, s[0:1], 0xa
	s_mov_b32 s4, SCRATCH_RSRC_DWORD0
	s_mov_b32 s5, SCRATCH_RSRC_DWORD1
	s_mov_b32 s6, -1
	s_mov_b32 s8, s3
	s_mov_b32 s7, 0xe8f000
	s_waitcnt lgkmcnt(0)
	v_mov_b32_e32 v0, s0
	buffer_load_dword v2, v0, s[4:7], s8 offen
	buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
	buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
	buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
	v_mov_b32_e32 v1, s2
	s_waitcnt vmcnt(0)
	buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
	buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
	buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
	buffer_store_dword v2, v1, s[4:7], s8 offen
	s_endpgm
.Lfunc_end0:
	.size	foo, .Lfunc_end0-foo

	.section	.AMDGPU.csdata

As you can see, assembly also has 4 load/store. I think gpu target does not handle it specially.

My guess here is that targets that do care are looking through the vector shuffle and customizing to whatever seems the best to them. If you wanna change the default behavior you need some data to show that your model solves a real issue and actually brings benefits; do you have any real examples on where that happens, or why GPU targets haven't yet tried to change this? Maybe other custom front-ends based on clang do? Finding the historical reason (if any) should be a good start point.

I did "git blame" and I read the commit's message. You can also see it with "c58dcdc8facb646d88675bb6fbcb5c787166c4be". It is same with clang code's comment. I also wonder how the vec3 --> vec4 load/store has not caused problems. As Akira's example, if struct type has float3 type as one of fields and it has 'packed' attribute, it overwrites next field. The vec3 load/store generates more instructions like stores and extract_vectors like below SelectionDAG.

LLVM IR for vec3

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %0 = load <3 x float>, <3 x float>* %b, align 16
  store <3 x float> %0, <3 x float>* %a, align 16
  ret void
}

SelectionDAG after type legalization for amdgcn (other targets has similar SelectionDAG after type legalization because of widen vector store on type legalizer)

Type-legalized selection DAG: BB#0 'foo:entry'
SelectionDAG has 24 nodes:
  t0: ch = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
    t4: i64 = add t2, Constant:i64<36>
  t6: i32,ch = load<LD4[undef(addrspace=2)](nontemporal)(dereferenceable)(invariant)> t0, t4, undef:i64
    t9: i64 = add t2, Constant:i64<40>
  t10: i32,ch = load<LD4[undef(addrspace=2)](nontemporal)(dereferenceable)(invariant)> t0, t9, undef:i64
    t12: ch = TokenFactor t6:1, t10:1
  t21: v4i32,ch = load<LD16[%b]> t12, t10, undef:i32
          t22: v2i64 = bitcast t21
        t24: i64 = extract_vector_elt t22, Constant:i32<0>
      t25: ch = store<ST8[%a](align=16)> t21:1, t24, t6, undef:i32
        t29: i32 = extract_vector_elt t21, Constant:i32<2>
        t27: i32 = add t6, Constant:i32<8>
      t30: ch = store<ST4[%a+8](align=8)> t21:1, t29, t27, undef:i32
    t33: ch = TokenFactor t25, t30
  t17: ch = ENDPGM t33

As you can see, the type legalizer handle vec3 load/store properly. It does not write 4th element. The vec3 load/store generates more instructions but it has correct behavior. I am not 100% sure the vec3 --> vec4 load/store is correct or not because no one has complained about it. But if the vec3 --> vec4 load/store is correct, llvm's type legalizer or somewhere on llvm's codegen could follow the approach too to generate optimal code. As a result, I think it would be good for clang to have both of features and I would like to stick to the option "-fpresereve-vec3' to change the behavior easily.

[Anastasia]

I don't think there is anything wrong with the generation of vec3->vec4 in Clang. I believe the motivation for this was the OpenCL spec treating vec3 as vec4 aligned type (see section 6.1.5: https://www.khronos.org/registry/OpenCL/specs/opencl-2.0-openclc.pdf#12). So in terms of memory layout vec3 wouldn't be any different to vec4. But in terms of operations (including loads/stores) there can be potential gain from not handling the 4th element. This can be exploited by some targets. I think generating the vec3 from the frontend would be a better choice in the first place. Because backend can decide how to handle this. Including for architectures with no SIMD support it would just generate 3 separate loads/stores. Right now it seems that it will be forced to generate 4 loads/stores.

But considering that transformation to vec4 has been the default implementation for quite a while in the frontend, I think we would need a stronger motivation for switching to original vec3. So current approach with a special flag for preserving vec3 should be good enough to fit all needs.

[jaykang10]

In D30810#701861, @Anastasia wrote:

I don't think there is anything wrong with the generation of vec3->vec4 in Clang. I believe the motivation for this was the OpenCL spec treating vec3 as vec4 aligned type (see section 6.1.5: https://www.khronos.org/registry/OpenCL/specs/opencl-2.0-openclc.pdf#12). So in terms of memory layout vec3 wouldn't be any different to vec4. But in terms of operations (including loads/stores) there can be potential gain from not handling the 4th element. This can be exploited by some targets. I think generating the vec3 from the frontend would be a better choice in the first place. Because backend can decide how to handle this. Including for architectures with no SIMD support it would just generate 3 separate loads/stores. Right now it seems that it will be forced to generate 4 loads/stores.

But considering that transformation to vec4 has been the default implementation for quite a while in the frontend, I think we would need a stronger motivation for switching to original vec3. So current approach with a special flag for preserving vec3 should be good enough to fit all needs.

Thank you, Anastasia. Can we go ahead and commit this patch?

[bruno]

As you can see, the type legalizer handle vec3 load/store properly. It does not write 4th element. The vec3 load/store generates more instructions but it has correct behavior. I am not 100% sure the vec3 --> vec4 load/store is correct or not because no one has complained about it. But if the vec3 --> vec4 load/store is correct, llvm's type legalizer or somewhere on llvm's codegen could follow the approach too to generate optimal code.

Thanks for the nice investigation/explanation on amdgcn.

As a result, I think it would be good for clang to have both of features and I would like to stick to the option "-fpresereve-vec3' to change the behavior easily.

The motivation doesn't seem solid to me, who else is going to benefit from this flag? You also didn't explain why doing this transformation yourself (looking through the shuffle) on your downstream pass isn't enough for you. We generally try to avoid adding flags if not for a good reason.

[jaykang10]

The motivation doesn't seem solid to me, who else is going to benefit from this flag? You also didn't explain why doing this transformation yourself (looking through the shuffle) on your downstream pass isn't enough for you. We generally try to avoid adding flags if not for a good reason.

If you turn on optimization with -O option, the shufflevector is gone on 'InstructionCombining' pass. I showed you the IR example and source code on previous comment. You could mention bitcast instead of shufflevector. The bitcast could be constant expression with global variables and it makes code dirty. I think Anastasia has already explained the motivation well on her comments. If you need something more, please let me know.

[ahatanak]

In D30810#701141, @jaykang10 wrote:

In D30810#701132, @ahatanak wrote:

Actually, it's not a mis-compile. The record layout shows that there is a padding before field f2 and f2 starts at byte 16. So using "store <4 x float>" doesn't overwrite the field.

It depends on float3's alignment. I guess the float3's alignment is 16 on your target so there is a padding bytes before f2 to be aligned by 16. If you add attribute((packed)) on struct type, you could see overwrite.

I looked at ItaniumRecordLayoutBuilder to understand how attribute((packed)) affects the struct's layout.

If the number of elements of a vector is not a power of 2, ASTContext::getTypeInfoImpl rounds up the width and alignment to the next power of 2. So the size of float3 is 16 bytes. attribute((packed)) can change the alignment of float3, but it doesn't change its size, so there is a 4-byte padding between field f1 and f2.

[Anastasia]

In D30810#702443, @bruno wrote:

As a result, I think it would be good for clang to have both of features and I would like to stick to the option "-fpresereve-vec3' to change the behavior easily.

The motivation doesn't seem solid to me, who else is going to benefit from this flag?

There are some off the main tree implementation that would benefit. But in the case of AMD GPU 3 loads/stores will be produced instead of 4. Sounds like a good optimization to me. As I said in my previous comment I think it should have been the default behavior from the beginning, but since different implementation landed first we can integrate this one now with an additional option.

[jaykang10]

In D30810#702548, @ahatanak wrote:

In D30810#701141, @jaykang10 wrote:

In D30810#701132, @ahatanak wrote:

Actually, it's not a mis-compile. The record layout shows that there is a padding before field f2 and f2 starts at byte 16. So using "store <4 x float>" doesn't overwrite the field.

It depends on float3's alignment. I guess the float3's alignment is 16 on your target so there is a padding bytes before f2 to be aligned by 16. If you add attribute((packed)) on struct type, you could see overwrite.

I looked at ItaniumRecordLayoutBuilder to understand how attribute((packed)) affects the struct's layout.

If the number of elements of a vector is not a power of 2, ASTContext::getTypeInfoImpl rounds up the width and alignment to the next power of 2. So the size of float3 is 16 bytes. attribute((packed)) can change the alignment of float3, but it doesn't change its size, so there is a 4-byte padding between field f1 and f2.

Oops, I am sorry, I was wrong. I did not imagine the size has same behavior with alignment. Thank you for pointing out it! :) Additionally, I wonder why the non-power-of-2 length vector's size has same behavior with alignment... Do you know something about the reason?

[jaykang10]

In D30810#702614, @Anastasia wrote:

In D30810#702443, @bruno wrote:

As a result, I think it would be good for clang to have both of features and I would like to stick to the option "-fpresereve-vec3' to change the behavior easily.

The motivation doesn't seem solid to me, who else is going to benefit from this flag?

There are some off the main tree implementation that would benefit. But in the case of AMD GPU 3 loads/stores will be produced instead of 4. Sounds like a good optimization to me. As I said in my previous comment I think it should have been the default behavior from the beginning, but since different implementation landed first we can integrate this one now with an additional option.

Additionally, Here is assembly output from vec3 with amdgcn target. :)

LLVM IR

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %0 = load <3 x float>, <3 x float>* %b, align 16
  store <3 x float> %0, <3 x float>* %a, align 16
  ret void
}

Assembly Output

	.text
	.section	.AMDGPU.config
	.long	47176
	.long	11272256
	.long	47180
	.long	132
	.long	47200
	.long	0
	.long	4
	.long	0
	.long	8
	.long	0
	.text
	.globl	foo
	.p2align	8
	.type	foo,@function
foo:                                    ; @foo
; BB#0:                                 ; %entry
	s_load_dword s2, s[0:1], 0x9
	s_load_dword s0, s[0:1], 0xa
	s_mov_b32 s4, SCRATCH_RSRC_DWORD0
	s_mov_b32 s5, SCRATCH_RSRC_DWORD1
	s_mov_b32 s6, -1
	s_mov_b32 s8, s3
	s_mov_b32 s7, 0xe8f000
	s_waitcnt lgkmcnt(0)
	v_mov_b32_e32 v0, s0
	buffer_load_dword v2, v0, s[4:7], s8 offen
	buffer_load_dword v3, v0, s[4:7], s8 offen offset:8
	buffer_load_dword v0, v0, s[4:7], s8 offen offset:4
	v_mov_b32_e32 v1, s2
	s_waitcnt vmcnt(0)
	buffer_store_dword v0, v1, s[4:7], s8 offen offset:4
	buffer_store_dword v2, v1, s[4:7], s8 offen
	buffer_store_dword v3, v1, s[4:7], s8 offen offset:8
	s_endpgm
.Lfunc_end0:
	.size	foo, .Lfunc_end0-foo

	.section	.AMDGPU.csdata
; Kernel info:
; codeLenInByte = 112
; NumSgprs: 9
; NumVgprs: 4
; FloatMode: 192
; IeeeMode: 1
; ScratchSize: 0
; LDSByteSize: 0 bytes/workgroup (compile time only)
; SGPRBlocks: 1
; VGPRBlocks: 0
; NumSGPRsForWavesPerEU: 9
; NumVGPRsForWavesPerEU: 4
; ReservedVGPRFirst: 0
; ReservedVGPRCount: 0
; COMPUTE_PGM_RSRC2:USER_SGPR: 2
; COMPUTE_PGM_RSRC2:TRAP_HANDLER: 0
; COMPUTE_PGM_RSRC2:TGID_X_EN: 1
; COMPUTE_PGM_RSRC2:TGID_Y_EN: 0
; COMPUTE_PGM_RSRC2:TGID_Z_EN: 0
; COMPUTE_PGM_RSRC2:TIDIG_COMP_CNT: 0

	.section	".note.GNU-stack"

[jaykang10]

In D30810#702637, @jaykang10 wrote:

In D30810#702614, @Anastasia wrote:

In D30810#702443, @bruno wrote:

As a result, I think it would be good for clang to have both of features and I would like to stick to the option "-fpresereve-vec3' to change the behavior easily.

The motivation doesn't seem solid to me, who else is going to benefit from this flag?

There are some off the main tree implementation that would benefit. But in the case of AMD GPU 3 loads/stores will be produced instead of 4. Sounds like a good optimization to me. As I said in my previous comment I think it should have been the default behavior from the beginning, but since different implementation landed first we can integrate this one now with an additional option.

Additionally, Here is assembly output from vec3 with amdgcn target. :)

LLVM IR

define void @foo(<3 x float>* nocapture %a, <3 x float>* nocapture readonly %b) {
entry:
  %0 = load <3 x float>, <3 x float>* %b, align 16
  store <3 x float> %0, <3 x float>* %a, align 16
  ret void
}

Assembly Output

	.text
	.section	.AMDGPU.config
	.long	47176
	.long	11272256
	.long	47180
	.long	132
	.long	47200
	.long	0
	.long	4
	.long	0
	.long	8
	.long	0
	.text
	.globl	foo
	.p2align	8
	.type	foo,@function
foo:                                    ; @foo
; BB#0:                                 ; %entry
	s_load_dword s2, s[0:1], 0x9
	s_load_dword s0, s[0:1], 0xa
	s_mov_b32 s4, SCRATCH_RSRC_DWORD0
	s_mov_b32 s5, SCRATCH_RSRC_DWORD1
	s_mov_b32 s6, -1
	s_mov_b32 s8, s3
	s_mov_b32 s7, 0xe8f000
	s_waitcnt lgkmcnt(0)
	v_mov_b32_e32 v0, s0
	buffer_load_dword v2, v0, s[4:7], s8 offen
	buffer_load_dword v3, v0, s[4:7], s8 offen offset:8
	buffer_load_dword v0, v0, s[4:7], s8 offen offset:4
	v_mov_b32_e32 v1, s2
	s_waitcnt vmcnt(0)
	buffer_store_dword v0, v1, s[4:7], s8 offen offset:4
	buffer_store_dword v2, v1, s[4:7], s8 offen
	buffer_store_dword v3, v1, s[4:7], s8 offen offset:8
	s_endpgm
.Lfunc_end0:
	.size	foo, .Lfunc_end0-foo

	.section	.AMDGPU.csdata
; Kernel info:
; codeLenInByte = 112
; NumSgprs: 9
; NumVgprs: 4
; FloatMode: 192
; IeeeMode: 1
; ScratchSize: 0
; LDSByteSize: 0 bytes/workgroup (compile time only)
; SGPRBlocks: 1
; VGPRBlocks: 0
; NumSGPRsForWavesPerEU: 9
; NumVGPRsForWavesPerEU: 4
; ReservedVGPRFirst: 0
; ReservedVGPRCount: 0
; COMPUTE_PGM_RSRC2:USER_SGPR: 2
; COMPUTE_PGM_RSRC2:TRAP_HANDLER: 0
; COMPUTE_PGM_RSRC2:TGID_X_EN: 1
; COMPUTE_PGM_RSRC2:TGID_Y_EN: 0
; COMPUTE_PGM_RSRC2:TGID_Z_EN: 0
; COMPUTE_PGM_RSRC2:TIDIG_COMP_CNT: 0

	.section	".note.GNU-stack"

Hi Anastasia, Bruno,

Do you still have other opinion? or Can we go ahead and commit this patch?

[Anastasia]

Hi Anastasia, Bruno,

Do you still have other opinion? or Can we go ahead and commit this patch?

I would like to see this patch committed. I see clear evidence of it improving existing GPU targets in the master repo as well as outside of the main tree implementations. Bruno, do you have any more concerns?

[Anastasia]

In D30810#699428, @Anastasia wrote:

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

Not sure it got lost somewhere... do you think you could address this too?

[Anastasia]

In D30810#706676, @Anastasia wrote:

In D30810#699428, @Anastasia wrote:

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

Not sure it got lost somewhere... do you think you could address this too?

I guess due to 4 element alignment the generated casts as vec4 should be fine for vec3, but it is worth checking...

[jaykang10]

In D30810#706677, @Anastasia wrote:

In D30810#706676, @Anastasia wrote:

In D30810#699428, @Anastasia wrote:

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

Not sure it got lost somewhere... do you think you could address this too?

I guess due to 4 element alignment the generated casts as vec4 should be fine for vec3, but it is worth checking...

Let's look at examples.

Source code

void kernel float4_to_float3(global float3 *a, global float4 *b) {
  //float4 --> float3
  *a = __builtin_astype(*b, float3);
}

void kernel float3_to_float4(global float3 *a, global float4 *b) {
  //float3 --> float4
  *b = __builtin_astype(*a, float4);
}

LLVM IR

; Function Attrs: norecurse nounwind
define void @float4_to_float3(<3 x float>* nocapture %a, <4 x float>* nocapture readonly %b) {
entry:
  %0 = load <4 x float>, <4 x float>* %b, align 16, !tbaa !6
  %storetmp = bitcast <3 x float>* %a to <4 x float>*
  store <4 x float> %0, <4 x float>* %storetmp, align 16, !tbaa !6
  ret void
}

; Function Attrs: norecurse nounwind
define void @float3_to_float4(<3 x float>* nocapture readonly %a, <4 x float>* nocapture %b) {
entry:
  %castToVec4 = bitcast <3 x float>* %a to <4 x float>*
  %loadVec4 = load <4 x float>, <4 x float>* %castToVec4, align 16
  store <4 x float> %loadVec4, <4 x float>* %b, align 16, !tbaa !6
  ret void
}

We could change above IR with vec3 as following:

; Function Attrs: norecurse nounwinddefine void @float4_to_float3(<3 x float>* nocapture %a, <4 x float>* nocapture readonly %b) {
entry:
  %0 = load <4 x float>, <4 x float>* %b, align 16
  %1 = shufflevector <4 x float> %0, <4 x float> undef, <3 x i32><i32 0, i32 1, i32 2>
  store <3 x float> %1, <3 x float>* %a, align 16
  ret void
}

; Function Attrs: norecurse nounwinddefine void @float3_to_float4(<3 x float>* nocapture readonly %a, <4 x float>* nocapture %b){
entry:
  %0 = load <3 x float>, <3 x float>* %a, align 16  %1 = shufflevector <3 x float> %0, <3 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
  store <4 x float> %1, <4 x float>* %b, align 16
  ret void
}

I guess it is what you want on LLVM IR. I have compiled above IRs with amdgcn target.

vec4 float4_to_float3 assembly code

float4_to_float3:                       ; @float4_to_float3
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9 
        s_load_dword s0, s[0:1], 0xa 
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s0
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        v_mov_b32_e32 v1, s2
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

vec3 float4_to_float3 assembly code

float4_to_float3:                       ; @float4_to_float3
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9 
        s_load_dword s0, s[0:1], 0xa 
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s0
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:4
        v_mov_b32_e32 v1, s2
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:8
        s_endpgm

I think it is what we expect because there are 3 load/store for vec instead of 4 load/store.

The float3_to_float4's output is different with float4_to_float3.

vec4 float3_to_float4 assembly code

float3_to_float4:                       ; @float3_to_float4
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s2
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        s_load_dword s0, s[0:1], 0xa
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v1, s0
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

vec4 float3_to_float4 assembly code

float3_to_float4:                       ; @float3_to_float4
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s2
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        s_load_dword s0, s[0:1], 0xa
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v1, s0
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

The output is same for float3_to_float4 between vec3 and vec4 because codegen legalizes the type with widen vector according to alignment. In float4_to_float3 case, codegen legalizes the store's vec3 type differently and in the end, it generates 3 load/store. As a result, I think we could generate above vec3 IR or similar one with option and it could be better output for GPU. On previous comments, I was wrong for "ScalarExprEmitter::VisitAsTypeExpr". I am sorry for that. If you agree with above vec3 IRs, I will add it on this patch. If I missed something or you have another idea or opinion, please let me know.

[Anastasia]

In D30810#707176, @jaykang10 wrote:

In D30810#706677, @Anastasia wrote:

In D30810#706676, @Anastasia wrote:

In D30810#699428, @Anastasia wrote:

Would you be able to update ScalarExprEmitter::VisitAsTypeExpr implementation accordingly to make things consistent?

Not sure it got lost somewhere... do you think you could address this too?

I guess due to 4 element alignment the generated casts as vec4 should be fine for vec3, but it is worth checking...

Let's look at examples.

Source code

void kernel float4_to_float3(global float3 *a, global float4 *b) {
  //float4 --> float3
  *a = __builtin_astype(*b, float3);
}

void kernel float3_to_float4(global float3 *a, global float4 *b) {
  //float3 --> float4
  *b = __builtin_astype(*a, float4);
}

LLVM IR

; Function Attrs: norecurse nounwind
define void @float4_to_float3(<3 x float>* nocapture %a, <4 x float>* nocapture readonly %b) {
entry:
  %0 = load <4 x float>, <4 x float>* %b, align 16, !tbaa !6
  %storetmp = bitcast <3 x float>* %a to <4 x float>*
  store <4 x float> %0, <4 x float>* %storetmp, align 16, !tbaa !6
  ret void
}

; Function Attrs: norecurse nounwind
define void @float3_to_float4(<3 x float>* nocapture readonly %a, <4 x float>* nocapture %b) {
entry:
  %castToVec4 = bitcast <3 x float>* %a to <4 x float>*
  %loadVec4 = load <4 x float>, <4 x float>* %castToVec4, align 16
  store <4 x float> %loadVec4, <4 x float>* %b, align 16, !tbaa !6
  ret void
}

We could change above IR with vec3 as following:

; Function Attrs: norecurse nounwinddefine void @float4_to_float3(<3 x float>* nocapture %a, <4 x float>* nocapture readonly %b) {
entry:
  %0 = load <4 x float>, <4 x float>* %b, align 16
  %1 = shufflevector <4 x float> %0, <4 x float> undef, <3 x i32><i32 0, i32 1, i32 2>
  store <3 x float> %1, <3 x float>* %a, align 16
  ret void
}

; Function Attrs: norecurse nounwinddefine void @float3_to_float4(<3 x float>* nocapture readonly %a, <4 x float>* nocapture %b){
entry:
  %0 = load <3 x float>, <3 x float>* %a, align 16  %1 = shufflevector <3 x float> %0, <3 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
  store <4 x float> %1, <4 x float>* %b, align 16
  ret void
}

I guess it is what you want on LLVM IR. I have compiled above IRs with amdgcn target.

vec4 float4_to_float3 assembly code

float4_to_float3:                       ; @float4_to_float3
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9 
        s_load_dword s0, s[0:1], 0xa 
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s0
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        v_mov_b32_e32 v1, s2
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

vec3 float4_to_float3 assembly code

float4_to_float3:                       ; @float4_to_float3
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9 
        s_load_dword s0, s[0:1], 0xa 
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s0
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:4
        v_mov_b32_e32 v1, s2
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:8
        s_endpgm

I think it is what we expect because there are 3 load/store for vec instead of 4 load/store.

The float3_to_float4's output is different with float4_to_float3.

vec4 float3_to_float4 assembly code

float3_to_float4:                       ; @float3_to_float4
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s2
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        s_load_dword s0, s[0:1], 0xa
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v1, s0
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

vec4 float3_to_float4 assembly code

float3_to_float4:                       ; @float3_to_float4
; BB#0:                                 ; %entry
        s_load_dword s2, s[0:1], 0x9
        s_mov_b32 s4, SCRATCH_RSRC_DWORD0
        s_mov_b32 s5, SCRATCH_RSRC_DWORD1
        s_mov_b32 s6, -1
        s_mov_b32 s8, s3
        s_mov_b32 s7, 0xe8f000
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v0, s2
        buffer_load_dword v2, v0, s[4:7], s8 offen
        buffer_load_dword v3, v0, s[4:7], s8 offen offset:4
        buffer_load_dword v4, v0, s[4:7], s8 offen offset:8
        buffer_load_dword v0, v0, s[4:7], s8 offen offset:12
        s_load_dword s0, s[0:1], 0xa
        s_waitcnt lgkmcnt(0)
        v_mov_b32_e32 v1, s0
        s_waitcnt vmcnt(0)
        buffer_store_dword v0, v1, s[4:7], s8 offen offset:12
        buffer_store_dword v4, v1, s[4:7], s8 offen offset:8
        buffer_store_dword v3, v1, s[4:7], s8 offen offset:4
        buffer_store_dword v2, v1, s[4:7], s8 offen
        s_endpgm

The output is same for float3_to_float4 between vec3 and vec4 because codegen legalizes the type with widen vector according to alignment. In float4_to_float3 case, codegen legalizes the store's vec3 type differently and in the end, it generates 3 load/store. As a result, I think we could generate above vec3 IR or similar one with option and it could be better output for GPU. On previous comments, I was wrong for "ScalarExprEmitter::VisitAsTypeExpr". I am sorry for that. If you agree with above vec3 IRs, I will add it on this patch. If I missed something or you have another idea or opinion, please let me know.

Yes. This would make sense. I am guessing that in vec3->vec4, we will have 3 loads and 4 stores and in vec4->vec3 we will have 4 loads and 3 stores?

Although, I am guessing the load/store optimizations would come from your current change? I don't see anything related to it in the VisitAsTypeExpr implementation. But I think it might be a good idea to add this to the test to make sure the IR output is as we expect.

[jaykang10]

Yes. This would make sense. I am guessing that in vec3->vec4, we will have 3 loads and 4 stores and in vec4->vec3 we will have 4 loads and 3 stores?

It depends on implementation. If you scalarize all vector operations on LLVM IR level before entering llvm's codegen, the vec3->vec4 would generate 3 loads and 4 stores and the vec4->vec3 would generate 4 loads and 3 stores. I guess your implementation follows this situation. The AMDGPU target keeps the vector form on LLVM IR level and handles it with legalization of SelectionDAG on llvm's codegen. On this situation, vec3->vec4 generates 4 loads and 4 stores because type legalizer widen vec3 load to vec4 load because the alignment is 16. vec4->vec3 generates 4 loads and 3 stores with type legalization. The output could be different according to llvm's backend implementation.

Although, I am guessing the load/store optimizations would come from your current change? I don't see anything related to it in the VisitAsTypeExpr implementation. But I think it might be a good idea to add this to the test to make sure the IR output is as we expect.

We should implement the IRs with option. I will update patch with it.

[jaykang10]

Preserved vec3 type on __builtin_astype.

[bruno]

I would like to see this patch committed. I see clear evidence of it improving existing GPU targets in the master repo as well as outside of the main tree implementations. Bruno, do you have any more concerns?

I believe the argument lacks numbers (or at least you have them but didn't mention). I didn't hear about performance results, or validation that this was actually tested for correctness. Small test cases prove a point, but can't be considered general.

OTOH, it seems like this is exactly why you want the flag, to hide any potential issues and play with it. I'm not opposed to adding the flag if there's a commitment to actually get the result and change the default to whatever seems to be better, does that seems reasonable?

[jaykang10]

I believe the argument lacks numbers (or at least you have them but didn't mention). I didn't hear about performance results, or validation that this was actually tested for correctness. Small test cases prove a point, but can't be considered general.

OTOH, it seems like this is exactly why you want the flag, to hide any potential issues and play with it. I'm not opposed to adding the flag if there's a commitment to actually get the result and change the default to whatever seems to be better, does that seems reasonable?

To be honest, I did not start this discussion to get better performance for GPU. But the goal has moved while we discussing it. I think I have showed you the gain from AMD GPU target on previous comments. On current version of clang/llvm, it generates one less load/store to preserve vec3 type instead of vec4 type on GPU target. But it could cause more load/store on another targets which have vector register because they usually expect aligned vector load/store. It means that if we want to change default to vec3 on clang, we need to change llvm's codegen's default legalization or backend targets need to be modified in order to get aligned vector load/store with vec3 type. I think It is too big change at this moment. In the future, we could move this discussion to llvm-dev. Up to that time, as you mentioned, I would like to hide the potential issues with vec3 type and play with it.

[Anastasia]

In D30810#710069, @jaykang10 wrote:

I believe the argument lacks numbers (or at least you have them but didn't mention). I didn't hear about performance results, or validation that this was actually tested for correctness. Small test cases prove a point, but can't be considered general.

OTOH, it seems like this is exactly why you want the flag, to hide any potential issues and play with it. I'm not opposed to adding the flag if there's a commitment to actually get the result and change the default to whatever seems to be better, does that seems reasonable?

To be honest, I did not start this discussion to get better performance for GPU. But the goal has moved while we discussing it. I think I have showed you the gain from AMD GPU target on previous comments. On current version of clang/llvm, it generates one less load/store to preserve vec3 type instead of vec4 type on GPU target. But it could cause more load/store on another targets which have vector register because they usually expect aligned vector load/store. It means that if we want to change default to vec3 on clang, we need to change llvm's codegen's default legalization or backend targets need to be modified in order to get aligned vector load/store with vec3 type. I think It is too big change at this moment. In the future, we could move this discussion to llvm-dev. Up to that time, as you mentioned, I would like to hide the potential issues with vec3 type and play with it.

Just to summarize. There seems to be no issue in adding this feature by a special flag considering that changing default behaviors requires changing the backend that we might consider at some later point of time. We can start a separate discussion on that.

The change can be used to generate more generic and optimal IR resulting in 1/4 less instructions generated in the final binary per each load and store of vec3 types in some architectures (e.g. AMD) . I am not sure what kind of further numbers are needed and if there is any measuring infrastructure LLVM project provides for that.

Considering all the above I don't feel there is any reason this can't be committed.

[Anastasia]

LGTM! Thanks!

[jaykang10]

In D30810#718176, @Anastasia wrote:

LGTM! Thanks!

Thanks Anastasia! Merge Done!

cfe/trunk/include/clang/Driver/CC1Options.td

 def fnative_half_arguments_and_returns : Flag<["-"], "fnative-half-arguments-and-returns">,
   HelpText<"Use the native __fp16 type for arguments and returns (and skip ABI-specific lowering)">;
 def fallow_half_arguments_and_returns : Flag<["-"], "fallow-half-arguments-and-returns">,
   HelpText<"Allow function arguments and returns of type half">;
 def fdefault_calling_conv_EQ : Joined<["-"], "fdefault-calling-conv=">,
   HelpText<"Set default MS calling convention">;
 def finclude_default_header : Flag<["-"], "finclude-default-header">,
   HelpText<"Include the default header file for OpenCL">;
+def fpreserve_vec3_type : Flag<["-"], "fpreserve-vec3-type">,
+  HelpText<"Preserve 3-component vector type">;
 
 // FIXME: Remove these entirely once functionality/tests have been excised.
 def fobjc_gc_only : Flag<["-"], "fobjc-gc-only">, Group<f_Group>,
   HelpText<"Use GC exclusively for Objective-C related memory management">;
 def fobjc_gc : Flag<["-"], "fobjc-gc">, Group<f_Group>,
   HelpText<"Enable Objective-C garbage collection">;
 
 //===----------------------------------------------------------------------===//

cfe/trunk/include/clang/Frontend/CodeGenOptions.def

 
 /// Whether we should use the undefined behaviour optimization for control flow
 /// paths that reach the end of a function without executing a required return.
 CODEGENOPT(StrictReturn, 1, 1)
 
 /// Whether emit extra debug info for sample pgo profile collection.
 CODEGENOPT(DebugInfoForProfiling, 1, 0)
 
+/// Whether 3-component vector type is preserved.
+CODEGENOPT(PreserveVec3Type, 1, 0)
+
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
 

cfe/trunk/lib/CodeGen/CGExpr.cpp

 llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
                                                QualType Ty,
                                                SourceLocation Loc,
                                                AlignmentSource AlignSource,
                                                llvm::MDNode *TBAAInfo,
                                                QualType TBAABaseType,
                                                uint64_t TBAAOffset,
                                                bool isNontemporal) {
+  if (!CGM.getCodeGenOpts().PreserveVec3Type) {
     // For better performance, handle vector loads differently.
     if (Ty->isVectorType()) {
       const llvm::Type *EltTy = Addr.getElementType();
 
       const auto *VTy = cast<llvm::VectorType>(EltTy);
 
       // Handle vectors of size 3 like size 4 for better performance.
       if (VTy->getNumElements() == 3) {
 
         // Bitcast to vec4 type.
-      llvm::VectorType *vec4Ty = llvm::VectorType::get(VTy->getElementType(),
-                                                         4);
+        llvm::VectorType *vec4Ty =
+            llvm::VectorType::get(VTy->getElementType(), 4);
         Address Cast = Builder.CreateElementBitCast(Addr, vec4Ty, "castToVec4");
         // Now load value.
         llvm::Value *V = Builder.CreateLoad(Cast, Volatile, "loadVec4");
 
         // Shuffle vector to get vec3.
         V = Builder.CreateShuffleVector(V, llvm::UndefValue::get(vec4Ty),
                                         {0, 1, 2}, "extractVec");
         return EmitFromMemory(V, Ty);
       }
     }
+  }
 
   // Atomic operations have to be done on integral types.
   LValue AtomicLValue =
       LValue::MakeAddr(Addr, Ty, getContext(), AlignSource, TBAAInfo);
   if (Ty->isAtomicType() || LValueIsSuitableForInlineAtomic(AtomicLValue)) {
     return EmitAtomicLoad(AtomicLValue, Loc).getScalarVal();
   }
 

 void CodeGenFunction::EmitStoreOfScalar(llvm::Value *Value, Address Addr,
                                         bool Volatile, QualType Ty,
                                         AlignmentSource AlignSource,
                                         llvm::MDNode *TBAAInfo,
                                         bool isInit, QualType TBAABaseType,
                                         uint64_t TBAAOffset,
                                         bool isNontemporal) {
 
+  if (!CGM.getCodeGenOpts().PreserveVec3Type) {
     // Handle vectors differently to get better performance.
     if (Ty->isVectorType()) {
       llvm::Type *SrcTy = Value->getType();
       auto *VecTy = cast<llvm::VectorType>(SrcTy);
       // Handle vec3 special.
       if (VecTy->getNumElements() == 3) {
         // Our source is a vec3, do a shuffle vector to make it a vec4.
         llvm::Constant *Mask[] = {Builder.getInt32(0), Builder.getInt32(1),
                                   Builder.getInt32(2),
                                   llvm::UndefValue::get(Builder.getInt32Ty())};
         llvm::Value *MaskV = llvm::ConstantVector::get(Mask);
-      Value = Builder.CreateShuffleVector(Value,
-                                          llvm::UndefValue::get(VecTy),
+        Value = Builder.CreateShuffleVector(Value, llvm::UndefValue::get(VecTy),
                                             MaskV, "extractVec");
         SrcTy = llvm::VectorType::get(VecTy->getElementType(), 4);
       }
       if (Addr.getElementType() != SrcTy) {
         Addr = Builder.CreateElementBitCast(Addr, SrcTy, "storetmp");
       }
     }
+  }
 
   Value = EmitToMemory(Value, Ty);
 
   LValue AtomicLValue =
       LValue::MakeAddr(Addr, Ty, getContext(), AlignSource, TBAAInfo);
   if (Ty->isAtomicType() ||
       (!isInit && LValueIsSuitableForInlineAtomic(AtomicLValue))) {
     EmitAtomicStore(RValue::get(Value), AtomicLValue, isInit);

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

unsigned NumElementsSrc = isa<llvm::VectorType>(SrcTy) ?
     cast<llvm::VectorType>(SrcTy)->getNumElements() : 0;
   unsigned NumElementsDst = isa<llvm::VectorType>(DstTy) ?
     cast<llvm::VectorType>(DstTy)->getNumElements() : 0;
 
   // Going from vec3 to non-vec3 is a special case and requires a shuffle
   // vector to get a vec4, then a bitcast if the target type is different.
   if (NumElementsSrc == 3 && NumElementsDst != 3) {
     Src = ConvertVec3AndVec4(Builder, CGF, Src, 4);
+
+    if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
       Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
                                          DstTy);
+    }
+
     Src->setName("astype");
     return Src;
   }
 
   // Going from non-vec3 to vec3 is a special case and requires a bitcast
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
+    if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
       auto Vec4Ty = llvm::VectorType::get(DstTy->getVectorElementType(), 4);
       Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
                                          Vec4Ty);
+    }
+
     Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
     Src->setName("astype");
     return Src;
   }
 
   return Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(),
                                             Src, DstTy, "astype");
 }

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

for (const auto &A : Args) {
         StringRef ArgStr(arg);
         Opts.CmdArgs.insert(Opts.CmdArgs.end(), ArgStr.begin(), ArgStr.end());
         // using \00 to seperate each commandline options.
         Opts.CmdArgs.push_back('\0');
       }
     }
   }
 
+  Opts.PreserveVec3Type = Args.hasArg(OPT_fpreserve_vec3_type);
   Opts.InstrumentFunctions = Args.hasArg(OPT_finstrument_functions);
   Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
   Opts.XRayInstructionThreshold =
       getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
   Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
   Opts.CallFEntry = Args.hasArg(OPT_mfentry);
   Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
   Opts.CompressDebugSections = Args.hasArg(OPT_compress_debug_sections);

cfe/trunk/test/CodeGenOpenCL/preserve_vec3.cl

+// RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown -fpreserve-vec3-type  | FileCheck %s
+
+typedef float float3 __attribute__((ext_vector_type(3)));
+typedef float float4 __attribute__((ext_vector_type(4)));
+
+void kernel foo(global float3 *a, global float3 *b) {
+  // CHECK: %[[LOAD_A:.*]] = load <3 x float>, <3 x float> addrspace(1)* %a
+  // CHECK: store <3 x float> %[[LOAD_A]], <3 x float> addrspace(1)* %b
+  *b = *a;
+}
+
+void kernel float4_to_float3(global float3 *a, global float4 *b) {
+  // CHECK: %[[LOAD_A:.*]] = load <4 x float>, <4 x float> addrspace(1)* %b, align 16
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x float> %[[LOAD_A]], <4 x float> undef, <3 x i32> <i32 0, i32 1, i32 2>
+  // CHECK: store <3 x float> %[[ASTYPE:.*]], <3 x float> addrspace(1)* %a, align 16
+  *a = __builtin_astype(*b, float3);
+}
+
+void kernel float3_to_float4(global float3 *a, global float4 *b) {
+  // CHECK: %[[LOAD_A:.*]] = load <3 x float>, <3 x float> addrspace(1)* %a, align 16
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x float> %[[LOAD_A]], <3 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
+  // CHECK: store <4 x float> %[[ASTYPE:.*]], <4 x float> addrspace(1)* %b, align 16
+  *b = __builtin_astype(*a, float4);
+}