Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

BTW, in response to your earlier concern about stage 3: you posted the series well in time for end of stage 1, so I think it can still go in during stage 3. Robin Dapp writes: > Hi Richard, > >> It's hard to judge this in isolation because it's not clear when >> and how the new arguments are goin

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Richard, > It's hard to judge this in isolation because it's not clear when > and how the new arguments are going to be used, but it seems OK > in principle. Do you still want: > > /* If earliest == jump, try to build the cmove insn directly. > This is helpful when combine has created

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Robin Dapp writes: > Hi Richard, > > after giving it a second thought, and seeing that most of the changes to > existing code are not strictly necessary anymore, I figured it could be > easier not changing the current control flow too much like in the > attached patch. > > The changes remaining

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Ping :) Not wanting to pester but I'd have hoped to get this into stage1 still as it helps performance on s390 in some cases. Expecting there will be some more reviewing rounds for the remaining patches, would I need to open a PR in order to be able to "officially" commit this in a later stage? Th

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Richard, after giving it a second thought, and seeing that most of the changes to existing code are not strictly necessary anymore, I figured it could be easier not changing the current control flow too much like in the attached patch. The changes remaining are to "outsource" the maybe_ex

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Robin Dapp writes: > Hi Richard, > >> (2) Insert: >> >> if (SUBREG_P (src)) >>src = SUBREG_REG (src); >> >> here. >> >> OK with those changes if that works. Let me know if they don't — >> I'll try to be quicker with the next review. > > thank you, this looks good in a first testsu

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Richard, (2) Insert: if (SUBREG_P (src)) src = SUBREG_REG (src); here. OK with those changes if that works. Let me know if they don't — I'll try to be quicker with the next review. thank you, this looks good in a first testsuite run on s390. If you have time, would you min

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Robin, Thanks for the update and sorry for the late response. Robin Dapp writes: > Hi Richard, > >> Don't we still need this code (without the REG_DEAD handling) for the >> case in which… >> >>> + /* As we are transforming >>> +if (x > y) >>> + { >>> +a = b; >>> +

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Richard, Don't we still need this code (without the REG_DEAD handling) for the case in which… + /* As we are transforming +if (x > y) + { +a = b; +c = d; + } +into + a = (x > y) ... + c = (x > y) ... + +

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Robin Dapp writes: > Hi Richard, > > thanks for going through the patch set. > >> A hash_set might be simpler, given that the code only enters insns >> for which the bool is false. “rtx_insn *” would be better than rtx. > > Right, changed that. > >> Do you mean the sets might be removed or that t

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Hi Richard, thanks for going through the patch set. A hash_set might be simpler, given that the code only enters insns for which the bool is false. “rtx_insn *” would be better than rtx. Right, changed that. Do you mean the sets might be removed or that the checks might be removed? It's

Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

Sorry for the slow review. Robin Dapp writes: > When if-converting multiple SETs and we encounter a swap-style idiom > > if (a > b) > { > tmp = c; // [1] > c = d; > d = tmp; > } > > ifcvt should not generate a conditional move for the instruction at > [1]. > > In ord