This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] combine: Do not combine moves from hard registers


Hi Segher,

I find a problem with your change to add make_more_copies.
I am investigating those regressions, a big amount of them are wrong code generation.

One problem is that, make_more_copies will split the assignment of fp to sfp.

From:
(insn 48 26 28 5 (set (reg/f:SI 102 sfp)
        (reg/f:SI 11 fp)) -1
To:
(insn 51 32 26 5 (set (reg:SI 117)
        (reg/f:SI 11 fp)) 646 {*arm_movsi_vfp}
     (expr_list:REG_EQUIV (reg/f:SI 11 fp)
        (nil)))
(insn 48 26 28 5 (set (reg/f:SI 102 sfp)
        (reg:SI 117)) 646 {*arm_movsi_vfp}
     (expr_list:REG_DEAD (reg:SI 117)
        (nil)))

The original rtx is generated by expand_builtin_setjmp_receiver to adjust the frame pointer.

And later in LRA, it will try to eliminate frame_pointer with hard frame pointer which is
defined the ELIMINABLE_REGS.

Your change split the insn into two.
This makes it doesn't match the "from" and "to" regs defined in ELIMINABLE_REGS.
The if statement to generate the adjustment insn is been skipt.
And the original instruction is just been deleted!



Probably, we don't want to split the move rtx if they are related to entries defined in ELIMINABLE_REGS?


Regards,
Renlin

On 10/24/2018 09:23 AM, Christophe Lyon wrote:
On Wed, 24 Oct 2018 at 00:26, Segher Boessenkool
<segher@kernel.crashing.org> wrote:

Hi Christophe,

On Tue, Oct 23, 2018 at 03:25:55PM +0200, Christophe Lyon wrote:
On Tue, 23 Oct 2018 at 14:29, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
On Tue, Oct 23, 2018 at 12:14:27PM +0200, Christophe Lyon wrote:
I have noticed many regressions on arm and aarch64 between 265366 and
265408 (this commit is 265398).

I bisected at least one to this commit on aarch64:
FAIL: gcc.dg/ira-shrinkwrap-prep-1.c scan-rtl-dump ira "Split
live-range of register"
The same test also regresses on arm.

Many targets also fail gcc.dg/ira-shrinkwrap-prep-2.c; these tests fail
when random things in the RTL change, apparently.

This is PR87708 now.

For a whole picture of all the regressions I noticed during these two
commits, have a look at:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/265408/report-build-info.html

No thanks.  I am not going to click on 111 links and whatever is behind
those.  Please summarise, like, what was the diff in test_summary, and
then dig down into individual tests if you want.  Or whatever else works
both for you and for me.  This doesn't work for me.

OK.... this is not very practical for me either. There were 25 commits between
the two validations being compared,
25-28 gcc tests regressed on aarch64, depending on the exact target
177-206 gcc tests regressed on arm*, 7-29 gfortran regressions on arm*
so I could have to run many bisects to make sure every regression is
caused by the same commit.

So many, ouch!  I didn't realise.

I've now got the results of validating your patch only, compared to the
previous revision, and it does cause all the regressions I noticed earlier.

Since these are all automated builds with everything discarded after
computing the regressions, it's quite time consuming to re-run the
tests manually on my side (probably at least as much as it is for you).

Running arm tests is very painful for me.  But you say this is on aarch64
as well, I didn't realise that either; aarch64 should be easy to test,
we have many reasonable aarch64 machines in the cfarm.

I know this doesn't answer your question, but I thought you could run aarch64
tests easily and that would be more efficient for the project that you
do it directly
without waiting for me to provide hardly little more information.

Well, I'm not too familiar with aarch64, so if you can say "this Z is a
pretty simple test that should do X but now does Y" that would be a huge
help :-)

Maybe this will answer your question better:
List of aarch64-linux-gnu regressions:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/265408/aarch64-none-linux-gnu/diff-gcc-rh60-aarch64-none-linux-gnu-default-default-default.txt
List of arm-none-linux-gnueabihf regressions:
(gcc) http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/265408/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a9-neon-fp16.txt
(gfortran) http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/265408/arm-none-linux-gnueabihf/diff-gfortran-rh60-arm-none-linux-gnueabihf-arm-cortex-a9-neon-fp16.txt

That may help yes, thanks!

To me it just highlights again that we need a validation system easier to
work with when we break something on a target we are not familiar with.

OTOH a patch like this is likely to break many target-specific tests, and
that should not prevent commiting it imnsho.  If it actively breaks things,
then of course it shouldn't go in as-is, or if it breaks bootstrap, etc.

I run post-commit validations as finely grained as possible with the CPU
resources I have access to, that's not enough and I think having a
developer-accessible gerrit+jenkins-like system would be very valuable
to test patches before commit. We have a prototype in Linaro, not
production-ready. But I guess that would be worth another
discussion thread :)

Yeah...  One when people have time for it ;-)


Segher


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]