Re: [PATCH] Vector mode addresses
On Thu, Jan 30, 2014 at 9:47 PM, Jakub Jelinek wrote: > On Thu, Jan 30, 2014 at 08:27:47PM +, Paulo Matos wrote: >> Yes, it looks strange but it was the way we came up with to >> implement an instruction that loads from a pair of addresses. >> >> From what I wrote previously to Richard. >> "We have an instruction that loads two 32 bit values into a lower >> and upper parts of a 64bit register using a base register and a 64 >> bit register used as a double index. >> So, >> r1 <- [r0, r2] >> means: >> low(r1) = [r0 + low(r2)] >> high(r1) = [r0 + high(r2)]" > > That sounds like gather instruction e.g. i?86 AVX2/AVX512F has. > I don't like using vector mode for the address though, i?86 uses UNSPECs and > IMNSHO you should too. Or express it as vec_concat of two MEM loads > where address of each one will be the base + vec_select of the vector > register. Yeah, I don't like it either. It opens a whole can of worms I think. Richard. > Jakub
Re: [PATCH] Vector mode addresses
On 30/01/14 20:47, Jakub Jelinek wrote: On Thu, Jan 30, 2014 at 08:27:47PM +, Paulo Matos wrote: Yes, it looks strange but it was the way we came up with to implement an instruction that loads from a pair of addresses. From what I wrote previously to Richard. "We have an instruction that loads two 32 bit values into a lower and upper parts of a 64bit register using a base register and a 64 bit register used as a double index. So, r1 <- [r0, r2] means: low(r1) = [r0 + low(r2)] high(r1) = [r0 + high(r2)]" That sounds like gather instruction e.g. i?86 AVX2/AVX512F has. I don't like using vector mode for the address though, i?86 uses UNSPECs and IMNSHO you should too. Or express it as vec_concat of two MEM loads where address of each one will be the base + vec_select of the vector register. Jakub I will take a closer look at our pattern and how i?86 implements it. Thanks for the advice. Paulo Matos
Re: [PATCH] Vector mode addresses
On Thu, Jan 30, 2014 at 08:27:47PM +, Paulo Matos wrote: > Yes, it looks strange but it was the way we came up with to > implement an instruction that loads from a pair of addresses. > > From what I wrote previously to Richard. > "We have an instruction that loads two 32 bit values into a lower > and upper parts of a 64bit register using a base register and a 64 > bit register used as a double index. > So, > r1 <- [r0, r2] > means: > low(r1) = [r0 + low(r2)] > high(r1) = [r0 + high(r2)]" That sounds like gather instruction e.g. i?86 AVX2/AVX512F has. I don't like using vector mode for the address though, i?86 uses UNSPECs and IMNSHO you should too. Or express it as vec_concat of two MEM loads where address of each one will be the base + vec_select of the vector register. Jakub
Re: [PATCH] Vector mode addresses
On 30/01/14 17:10, Richard Sandiford wrote: Right, in the context: Just in case: the point I was trying to make in the second paragraph was that the code in the patch only triggers (as you say) because the address isn't seen as mode-dependent. But this kind of address does look mode-dependent to me, since it only works for MEM modes that have the same number of elements as the index vector. So this does sound like a case where mode_dependent_address_p ought to return true. If we do support vector addresses than I think the right fix is to check VECTOR_MODE_P there. http://gcc.gnu.org/ml/gcc/2014-01/msg00242.html I.e. there == mode_dependent_address_p (defined in recog.c). From this I understood that you agreed that if vector_mode is supported for an address the check should be in simplify_rtx as it would prevent all target ports from adding the check to their hook, making it therefore more generic. You re-enforced this when you said: "I'd just prefer it to be in generic code because I think it's a target-independent rule." No, I meant that I'd prefer it to be done in the target-independent mode_dependent_address_p function. This was in response to: Well, isn't it the case that a mem with vector mode is always mode dependent, in which case adding VECTOR_MODE_P to mode-dependent target hook would be enough making the patch not so useful? http://gcc.gnu.org/ml/gcc/2014-01/msg00248.html where it sounded like you were instead going to add the check to your target's TARGET_MODE_DEPENDENT_ADDRESS_P hook. I don't think it makes sense to defer the VECTOR_MODE_P check to the target hook since I don't know how target-independent code could attach any meaning to something like (mem:V4HI (reg:V4SI ...)) -> (mem:DI (reg:V4SI ...)). Again, this is all on the basis that vector-mode addresses really are supported. Now I understand what you mean. I was pretty confused by what you meant by target-independent mode_dependent_address_p because initially I had the feeling that targetm.mode_dependent_address_p was being called in simplify_rtx but there's actually a mode_dependent_address_p in recog.c and there is where you suggested to add the check _if_ vector modes are supported. Got it. I apologize for my misunderstanding and thank you for your patience. -- Paulo Matos Thanks, Richard
Re: [PATCH] Vector mode addresses
On 30/01/14 14:01, Richard Biener wrote: I'm curious on where you are seeing MEMs with a vector mode address. What does that MEM even mean? Yes, it looks strange but it was the way we came up with to implement an instruction that loads from a pair of addresses. From what I wrote previously to Richard. "We have an instruction that loads two 32 bit values into a lower and upper parts of a 64bit register using a base register and a 64 bit register used as a double index. So, r1 <- [r0, r2] means: low(r1) = [r0 + low(r2)] high(r1) = [r0 + high(r2)]" From the referenced mail: new_rtx: (mem:V4SI (plus:V4SI (vec_concat:V4SI (vec_concat:V2SI (const_int 0 [0]) (const_int 0 [0])) (vec_concat:V2SI (const_int 0 [0]) (const_int 0 [0]))) that should be invalid and somehow lacks the subreg:DI. The bug is where that got lost. I don't think it got lost. GCC was trying to simplify it. That's why my patch was in simplify_subreg. GCC was trying to simplify a subreg in DImode with this mem rtx as SUBREG_REG and offset 8. -- Paulo Matos
Re: [PATCH] Vector mode addresses
Paulo Matos writes: >> -Original Message- >> From: Richard Sandiford [mailto:rdsandif...@googlemail.com] >> Sent: 30 January 2014 12:43 >> To: Paulo Matos >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH] Vector mode addresses >> >> Paulo Matos writes: >> > As a followup of the thread: >> > http://gcc.gnu.org/ml/gcc/2014-01/msg00206.html >> > >> > consider the attached patch for submission. It fixes an ICE in case >> > you try to use vector mode addresses and do not have it as mode >> > dependent target hook. As discussed in the thread adding >> > VECTOR_MODE_P to the target hook is a way to avoid ICE but as Richard >> > S. mentioned it's more general to patch GCC up. >> >> But like I said, I think the VECTOR_MODE_P check should be in >> mode_dependent_address_p (recog.c) rather than here. If vector >> addresses are supported then they are mode-dependent, since the >> number of elements in the address mode must match the number of >> elements in the MEM mode. > > > Have I misunderstood what you said: > "If we do support vector addresses than I think the right fix is to > check VECTOR_MODE_P there." Right, in the context: Just in case: the point I was trying to make in the second paragraph was that the code in the patch only triggers (as you say) because the address isn't seen as mode-dependent. But this kind of address does look mode-dependent to me, since it only works for MEM modes that have the same number of elements as the index vector. So this does sound like a case where mode_dependent_address_p ought to return true. If we do support vector addresses than I think the right fix is to check VECTOR_MODE_P there. http://gcc.gnu.org/ml/gcc/2014-01/msg00242.html I.e. there == mode_dependent_address_p (defined in recog.c). > From this I understood that you agreed that if vector_mode is supported > for an address the check should be in simplify_rtx as it would prevent > all target ports from adding the check to their hook, making it > therefore more generic. You re-enforced this when you said: > "I'd just prefer it > to be in generic code because I think it's a target-independent rule." No, I meant that I'd prefer it to be done in the target-independent mode_dependent_address_p function. This was in response to: Well, isn't it the case that a mem with vector mode is always mode dependent, in which case adding VECTOR_MODE_P to mode-dependent target hook would be enough making the patch not so useful? http://gcc.gnu.org/ml/gcc/2014-01/msg00248.html where it sounded like you were instead going to add the check to your target's TARGET_MODE_DEPENDENT_ADDRESS_P hook. I don't think it makes sense to defer the VECTOR_MODE_P check to the target hook since I don't know how target-independent code could attach any meaning to something like (mem:V4HI (reg:V4SI ...)) -> (mem:DI (reg:V4SI ...)). Again, this is all on the basis that vector-mode addresses really are supported. Thanks, Richard
Re: [PATCH] Vector mode addresses
On Thu, Jan 30, 2014 at 1:51 PM, Paulo Matos wrote: >> -Original Message- >> From: Richard Sandiford [mailto:rdsandif...@googlemail.com] >> Sent: 30 January 2014 12:43 >> To: Paulo Matos >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH] Vector mode addresses >> >> Paulo Matos writes: >> > As a followup of the thread: >> > http://gcc.gnu.org/ml/gcc/2014-01/msg00206.html >> > >> > consider the attached patch for submission. It fixes an ICE in case >> > you try to use vector mode addresses and do not have it as mode >> > dependent target hook. As discussed in the thread adding >> > VECTOR_MODE_P to the target hook is a way to avoid ICE but as Richard >> > S. mentioned it's more general to patch GCC up. >> >> But like I said, I think the VECTOR_MODE_P check should be in >> mode_dependent_address_p (recog.c) rather than here. If vector >> addresses are supported then they are mode-dependent, since the >> number of elements in the address mode must match the number of >> elements in the MEM mode. > > > Have I misunderstood what you said: > "If we do support vector addresses than I think the right fix is to > check VECTOR_MODE_P there." > > From this I understood that you agreed that if vector_mode is supported for > an address the check should be in simplify_rtx as it would prevent all target > ports from adding the check to their hook, making it therefore more generic. > You re-enforced this when you said: > "I'd just prefer it > to be in generic code because I think it's a target-independent rule." > > Apologies if I completely misunderstood you. I'm curious on where you are seeing MEMs with a vector mode address. What does that MEM even mean? From the referenced mail: new_rtx: (mem:V4SI (plus:V4SI (vec_concat:V4SI (vec_concat:V2SI (const_int 0 [0]) (const_int 0 [0])) (vec_concat:V2SI (const_int 0 [0]) (const_int 0 [0]))) that should be invalid and somehow lacks the subreg:DI. The bug is where that got lost. Richard. >> >> Thanks, >> Richard
RE: [PATCH] Vector mode addresses
> -Original Message- > From: Richard Sandiford [mailto:rdsandif...@googlemail.com] > Sent: 30 January 2014 12:43 > To: Paulo Matos > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] Vector mode addresses > > Paulo Matos writes: > > As a followup of the thread: > > http://gcc.gnu.org/ml/gcc/2014-01/msg00206.html > > > > consider the attached patch for submission. It fixes an ICE in case > > you try to use vector mode addresses and do not have it as mode > > dependent target hook. As discussed in the thread adding > > VECTOR_MODE_P to the target hook is a way to avoid ICE but as Richard > > S. mentioned it's more general to patch GCC up. > > But like I said, I think the VECTOR_MODE_P check should be in > mode_dependent_address_p (recog.c) rather than here. If vector > addresses are supported then they are mode-dependent, since the > number of elements in the address mode must match the number of > elements in the MEM mode. Have I misunderstood what you said: "If we do support vector addresses than I think the right fix is to check VECTOR_MODE_P there." From this I understood that you agreed that if vector_mode is supported for an address the check should be in simplify_rtx as it would prevent all target ports from adding the check to their hook, making it therefore more generic. You re-enforced this when you said: "I'd just prefer it to be in generic code because I think it's a target-independent rule." Apologies if I completely misunderstood you. > > Thanks, > Richard
Re: [PATCH] Vector mode addresses
Paulo Matos writes: > As a followup of the thread: > http://gcc.gnu.org/ml/gcc/2014-01/msg00206.html > > consider the attached patch for submission. It fixes an ICE in case > you try to use vector mode addresses and do not have it as mode > dependent target hook. As discussed in the thread adding > VECTOR_MODE_P to the target hook is a way to avoid ICE but as Richard > S. mentioned it's more general to patch GCC up. But like I said, I think the VECTOR_MODE_P check should be in mode_dependent_address_p (recog.c) rather than here. If vector addresses are supported then they are mode-dependent, since the number of elements in the address mode must match the number of elements in the MEM mode. Thanks, Richard
