On Tue, Jan 26, 2016 at 05:39:24PM +0000, Wilco Dijkstra wrote: > ping (note the regressions discussed below are addressed by > https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01761.html)
OK, but please be extra vigilant for any fallout on AArch64 after this and the follow-up linked above is applied. Thanks, James > James Greenhalgh wrote: > > On Wed, Dec 16, 2015 at 01:05:21PM +0000, Wilco Dijkstra wrote: > > > James Greenhalgh wrote: > > > > On Tue, Dec 15, 2015 at 10:54:49AM +0000, Wilco Dijkstra wrote: > > > > > ping > > > > > > > > > > > -----Original Message----- > > > > > > From: Wilco Dijkstra [mailto:wilco.dijks...@arm.com] > > > > > > Sent: 06 November 2015 20:06 > > > > > > To: 'gcc-patches@gcc.gnu.org' > > > > > > Subject: [PATCH][AArch64] Add TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS > > > > > > > > > > > > This patch adds support for the > > > > > > TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS > > > > > > hook. When the cost of GENERAL_REGS and FP_REGS is identical, the > > > > > > register > > > > > > allocator always uses ALL_REGS even when it has a much higher cost. > > > > > > The > > > > > > hook changes the class to either FP_REGS or GENERAL_REGS depending > > > > > > on the > > > > > > mode of the register. This results in better register allocation > > > > > > overall, > > > > > > fewer spills and reduced codesize - particularly in SPEC2006 gamess. > > > > > > > > > > > > GCC regression passes with several minor fixes. > > > > > > > > > > > > OK for commit? > > > > > > > > > > > > ChangeLog: > > > > > > 2015-11-06 Wilco Dijkstra <wdijk...@arm.com> > > > > > > > > > > > > * gcc/config/aarch64/aarch64.c > > > > > > (TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS): New define. > > > > > > (aarch64_ira_change_pseudo_allocno_class): New function. > > > > > > * gcc/testsuite/gcc.target/aarch64/cvtf_1.c: Build with -O2. > > > > > > * gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c > > > > > > (test_corners_sisd_di): Improve force to SIMD register. > > > > > > (test_corners_sisd_si): Likewise. > > > > > > * gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c: Build with > > > > > > -O2. > > > > > > * gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c: > > > > > > Remove scan-assembler check for ldr. > > > > > > > > Drop the gcc/ from the ChangeLog. > > > > > > > > > > -- > > > > > > gcc/config/aarch64/aarch64.c | 22 > > > > > > ++++++++++++++++++++++ > > > > > > gcc/testsuite/gcc.target/aarch64/cvtf_1.c | 2 +- > > > > > > gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c | 4 ++-- > > > > > > gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c | 2 +- > > > > > > .../gcc.target/aarch64/vect-ld1r-compile-fp.c | 1 - > > > > > > > > These testsuite changes concern me a bit, and you don't mention them > > > > beyond > > > > saying they are minor fixes... > > > > > > Well any changes to register allocator preferencing would cause fallout in > > > tests that are assuming which register is allocated, especially if they > > > use > > > nasty inline assembler hacks to do so... > > > > Sure, but the testcases here each operate on data that should live in > > FP_REGS given the initial conditions that the nasty hacks try to mimic - > > that's what makes the regressions notable. > > > > > > > > > > > #define FCVTDEF(ftype,itype) \ > > > > > > void \ > > > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c > > > > > > b/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c > > > > > > index 363f554..8465c89 100644 > > > > > > --- a/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c > > > > > > +++ b/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c > > > > > > @@ -186,9 +186,9 @@ test_corners_sisd_di (Int64x1 b) > > > > > > { > > > > > > force_simd_di (b); > > > > > > b = b >> 63; > > > > > > + force_simd_di (b); > > > > > > b = b >> 0; > > > > > > b += b >> 65; /* { dg-warning "right shift count >= width of > > > > > > type" } */ > > > > > > - force_simd_di (b); > > > > > > > > This one I don't understand, but seems to say that we've decided to move > > > > b out of FP_REGS after getting it in there for b = b << 63; ? So this is > > > > another register allocator regression? > > > > > > No, basically the register allocator is now making better decisions as to > > > where to allocate integer variables. It will only allocate them to FP > > > registers if they are primarily used by other FP operations. The > > > force_simd_di inline assembler tries to mimic FP uses, and if there are > > > enough of them at the right places then everything works as expected. If > > > however you do 3 consecutive integer operations then the allocator will > > > now > > > correctly prefer to allocate them to the integer registers (while > > > previously > > > it wouldn't, which is inefficient). > > > > I'm not sure I understand this argument in the abstract (though I believe > > it for some of the supported cores for the AArch64 target). At an abstract > > level, given a set of operations which can execute in either FP_REGS or > > GENERAL_REGS and initial and post conditions that allocate all input and > > output registers from those operations to FP_REGS, I would expect those > > operations to take place using FP_REGS? Your patch seems to break this > > expectation? > > No my patch doesn't break that expectation. The goal is that if the cost of > allocating to either integer or FP registers is the same, we prefer the most > natural register file based on the type. We'll continue to allocate integer > operations to FP_REGS if that has the lowest cost. > > Like I mentioned in the explanation, the issue is that the register allocator > simply > ignores the the much higher cost of ALL_REGS and uses it eventhough it > results in > very suboptimal allocations and a large number of redundant int<->fp moves. > This patch fixes this by forcing the preference to FP_REGS or GENERAL_REGS if > it > Is ALL_REGS. > > > > > > > diff --git a/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c > > > > > > b/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c > > > > > > index a49db3e..c5a9c52 100644 > > > > > > --- a/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c > > > > > > +++ b/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c > > > > > > @@ -1,6 +1,6 @@ > > > > > > /* Test vdup_lane intrinsics work correctly. */ > > > > > > /* { dg-do run } */ > > > > > > -/* { dg-options "-O1 --save-temps" } */ > > > > > > +/* { dg-options "-O2 --save-temps" } */ > > > > > > > > Another -O1 regression ? > > > > > > No, it's triggering a bug in the -O1 register preferencing that causes > > > incorrect preferences to be > > > selected despite the costs being right. The cost calculation with -O1 for > > > eg. > > > wrap_vdupb_lane_s8_0() in vdup_lane_2.c: > > > > > > Pass 0 for finding pseudo/allocno costs > > > > > > r79: preferred FP_REGS, alternative GENERAL_REGS, allocno GENERAL_REGS > > > a1 (r79,l0) best GENERAL_REGS, allocno GENERAL_REGS > > > r78: preferred GENERAL_REGS, alternative NO_REGS, allocno GENERAL_REGS > > > a0 (r78,l0) best GENERAL_REGS, allocno GENERAL_REGS > > > > > > a0(r78,l0) costs: CALLER_SAVE_REGS:5000,5000 GENERAL_REGS:5000,5000 > > > FP_LO_REGS:5000,5000 FP_REGS:5000,5000 > > ALL_REGS:10000,10000 MEM:9000,9000 > > > a1(r79,l0) costs: CALLER_SAVE_REGS:5000,5000 GENERAL_REGS:5000,5000 > > > FP_LO_REGS:0,0 FP_REGS:0,0 ALL_REGS:10000,10000 > > MEM:9000,9000 > > > > > > So it correctly prefers FP_REGS for r79 as it has the lowest cost, but > > > then > > > forces the allocno and best register to GENERAL_REGS... We could work > > > around > > > it by not having the "r" variant first in the aarch64_get_lane patterns > > > and > > > further discouraging its use via "?r", but that's a different patch. > > > > Well, that patch (moving "r" alternative away from first) does seem to > > better fit with what we've done elsewhere in aarch64-simd.md (e.g. > > aarch64_combinez below). Does making this change obviate the need to > > change these testcases to -O1? If so, I'd rather break them with your patch > > and fix it in a follow-up than paper over the cracks. > > Yes, using "?r" works. I can easily add this to my combinez patch - the issue > is that there are a > lot more patterns that have the same problem, so we also need a fix in the > register allocator > (we need to do both as reload also has bugs where it completely ignores all > the costs and > preferences, so the order really matters a lot...). > > So I looked a bit further, and the bug is that the preferencing also forces > ALL_REGS if the > GENERAL_REGS and FP_REGS costs are not equal but both are lower than the > memory cost > (again even if ALL_REGS cost is higher than the memory cost!). > > In that case TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS will force the preference > irrespectively of the best preference. To fix this we need to extend it with > the best register > class (and possibly alternate class) so we can avoid forcing the wrong > preference if there already > is a good preference (ie. not ALL_REGS). I'll write a patch for that - it's > trivial but presumably too > late for GCC6 as it affects a target callback... > > Wilco
