* [0/8] Add optabs alternatives for insv, extv and extzv
@ 2012-11-03 11:10 Richard Sandiford
2012-11-03 11:13 ` [1/8] Handle TRUNCATE in make_extraction Richard Sandiford
` (8 more replies)
0 siblings, 9 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:10 UTC (permalink / raw)
To: gcc-patches
This series was inspired by Andrew's patch from September:
http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00126.html
and by a general frustration with the insv, extv and extzv interface.
The patches add 6 new optabs:
insvM: an M<-M register insertion
insvmisalignM: a BLK<-M memory insertion
extvM: a signed M<-M register extraction
extvmisalignM: a signed M<-BLK memory extraction
extzvM: an unsigned M<-M register extraction
extzvmisalignM: an unsigned M<-BLK memory extraction
The BLKmode memory references are to the full bitfield, instead of
just a byte_mode memory reference to the first byte. I used "misalign"
to emphasise that there's no alignment guarantee for the memory operand.
I've tried to leave open the possibility of having (aligned) M<-M memory
operations in future, but I haven't added them here because I have no
use case. It's also not clear to me whether we'd want to generate the
memory form during expand or leave it up to combine.
The main target of this change is MIPS64r2, which can do both 32-bit and
64-bit insertions and extractions. The current rtl code tends to do things
in 64 bits, because that's how the insv and ext(z)v interface is defined,
but doing things in 64 bits requires any 32-bit results to be explicitly
"truncated" (i.e. sign-extended). Providing the optabs interface makes
both widths available.
Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
Also tested by making sure that there were no changes in assembly
output for a set of gcc .ii files. On the other hand, the -march=octeon
output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
kicking in as hoped.
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* [1/8] Handle TRUNCATE in make_extraction
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
@ 2012-11-03 11:13 ` Richard Sandiford
2012-11-10 15:52 ` Eric Botcazou
2012-11-03 11:14 ` [2/8] Add adjust_bitfield_address_size Richard Sandiford
` (7 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:13 UTC (permalink / raw)
To: gcc-patches
combine.c:make_extraction peels off truncating lowpart subregs
from the structure operand. On !TRULY_NOOP_TRUNCATION targets,
truncations are represented as TRUNCATE instead, so this patch
handles them too.
As for why this patch is suddenly needed now: at the moment, we start
out with INNER being the original (truncate:SI (foo:DI)) expression,
but after having looked at the insv/ext(z)v pattern, we decide that
we want INNER to be word_mode (DImode) instead. The truncation then
gets stripped by the force_to_mode call.
However, with optabs, an SImode INNER would cause us to use an SImode
operation instead. That isn't what we want, because the explicit
TRUNCATE is there precisely because the DImode value needs to be
modified by active instructions before being acceptable in an
SImode operation.
Tested as described in the covering note. OK to install?
Richard
gcc/
* combine.c (make_extraction): Handle TRUNCATEd INNERs.
Index: gcc/combine.c
===================================================================
--- gcc/combine.c 2012-11-02 08:15:57.000000000 +0000
+++ gcc/combine.c 2012-11-02 08:22:07.702369220 +0000
@@ -7022,6 +7022,8 @@ make_extraction (enum machine_mode mode,
if (new_rtx != 0)
return gen_rtx_ASHIFT (mode, new_rtx, XEXP (inner, 1));
}
+ else if (GET_CODE (inner) == TRUNCATE)
+ inner = XEXP (inner, 0);
inner_mode = GET_MODE (inner);
^ permalink raw reply [flat|nested] 42+ messages in thread
* [2/8] Add adjust_bitfield_address_size
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
2012-11-03 11:13 ` [1/8] Handle TRUNCATE in make_extraction Richard Sandiford
@ 2012-11-03 11:14 ` Richard Sandiford
2012-11-10 15:53 ` Eric Botcazou
2012-11-03 11:16 ` [3/8] Add narrow_bit_field_mem Richard Sandiford
` (6 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:14 UTC (permalink / raw)
To: gcc-patches
As mentioned in the covering note, the new memory optabs take a BLKmode
reference to the bitfield. That requires a new adjust_bitfield_address
interface that takes the size of the reference as a parameter.
Tested as described in the covering note. OK to install?
Richard
gcc/
* expr.h (adjust_address_1): Add a size parameter.
(adjust_address, adjust_address_nv, adjust_bitfield_address)
(adjust_bitfield_address_nv): Adjust accordingly.
(adjust_bitfield_address_size): Define.
* emit-rtl.c (adjust_address_1): Add a size parameter.
Use it to set the size if MODE has no size. Check whether
the size matches before returning the original memref.
Require the size to be known for adjust_object.
(adjust_automodify_address_1, widen_memory_access): Update calls
to adjust_address_1.
Index: gcc/expr.h
===================================================================
--- gcc/expr.h 2012-11-01 23:06:58.000000000 +0000
+++ gcc/expr.h 2012-11-01 23:12:39.719369067 +0000
@@ -557,22 +557,27 @@ extern rtx change_address (rtx, enum mac
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address offset by OFFSET bytes. */
#define adjust_address(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0, 0)
/* Likewise, but the reference is not required to be valid. */
#define adjust_address_nv(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0, 0)
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address offset by OFFSET bytes. Assume that it's
for a bitfield and conservatively drop the underlying object if we
cannot be sure to stay within its bounds. */
#define adjust_bitfield_address(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, 0)
+
+/* As for adjust_bitfield_address, but specify that the width of
+ BLKmode accesses is SIZE bytes. */
+#define adjust_bitfield_address_size(MEMREF, MODE, OFFSET, SIZE) \
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, SIZE)
/* Likewise, but the reference is not required to be valid. */
#define adjust_bitfield_address_nv(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1, 0)
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address changed to ADDR, which is assumed to be
@@ -585,7 +590,7 @@ #define adjust_automodify_address_nv(MEM
adjust_automodify_address_1 (MEMREF, MODE, ADDR, OFFSET, 0)
extern rtx adjust_address_1 (rtx, enum machine_mode, HOST_WIDE_INT, int, int,
- int);
+ int, HOST_WIDE_INT);
extern rtx adjust_automodify_address_1 (rtx, enum machine_mode, rtx,
HOST_WIDE_INT, int);
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2012-11-01 23:06:58.000000000 +0000
+++ gcc/emit-rtl.c 2012-11-01 23:15:35.968368637 +0000
@@ -2061,11 +2061,14 @@ change_address (rtx memref, enum machine
If ADJUST_OBJECT is zero, the underlying object associated with the
memory reference is left unchanged and the caller is responsible for
dealing with it. Otherwise, if the new memory reference is outside
- the underlying object, even partially, then the object is dropped. */
+ the underlying object, even partially, then the object is dropped.
+ SIZE, if nonzero, is the size of an access in cases where MODE
+ has no inherent size. */
rtx
adjust_address_1 (rtx memref, enum machine_mode mode, HOST_WIDE_INT offset,
- int validate, int adjust_address, int adjust_object)
+ int validate, int adjust_address, int adjust_object,
+ HOST_WIDE_INT size)
{
rtx addr = XEXP (memref, 0);
rtx new_rtx;
@@ -2076,8 +2079,14 @@ adjust_address_1 (rtx memref, enum machi
attrs = *get_mem_attrs (memref);
+ /* Take the size of non-BLKmode accesses from the mode. */
+ defattrs = mode_mem_attrs[(int) mode];
+ if (defattrs->size_known_p)
+ size = defattrs->size;
+
/* If there are no changes, just return the original memory reference. */
if (mode == GET_MODE (memref) && !offset
+ && (size == 0 || (attrs.size_known_p && attrs.size == size))
&& (!validate || memory_address_addr_space_p (mode, addr,
attrs.addrspace)))
return memref;
@@ -2150,24 +2159,23 @@ adjust_address_1 (rtx memref, enum machi
attrs.align = MIN (attrs.align, max_align);
}
- /* We can compute the size in a number of ways. */
- defattrs = mode_mem_attrs[(int) GET_MODE (new_rtx)];
- if (defattrs->size_known_p)
+ if (size)
{
/* Drop the object if the new right end is not within its bounds. */
- if (adjust_object && (offset + defattrs->size) > attrs.size)
+ if (adjust_object && (offset + size) > attrs.size)
{
attrs.expr = NULL_TREE;
attrs.alias = 0;
}
attrs.size_known_p = true;
- attrs.size = defattrs->size;
+ attrs.size = size;
}
else if (attrs.size_known_p)
{
+ gcc_assert (!adjust_object);
attrs.size -= offset;
- /* ??? The store_by_pieces machinery generates negative sizes. */
- gcc_assert (!(adjust_object && attrs.size < 0));
+ /* ??? The store_by_pieces machinery generates negative sizes,
+ so don't assert for that here. */
}
set_mem_attrs (new_rtx, &attrs);
@@ -2185,7 +2193,7 @@ adjust_automodify_address_1 (rtx memref,
HOST_WIDE_INT offset, int validate)
{
memref = change_address_1 (memref, VOIDmode, addr, validate);
- return adjust_address_1 (memref, mode, offset, validate, 0, 0);
+ return adjust_address_1 (memref, mode, offset, validate, 0, 0, 0);
}
/* Return a memory reference like MEMREF, but whose address is changed by
@@ -2267,7 +2275,7 @@ replace_equiv_address_nv (rtx memref, rt
rtx
widen_memory_access (rtx memref, enum machine_mode mode, HOST_WIDE_INT offset)
{
- rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0);
+ rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0, 0);
struct mem_attrs attrs;
unsigned int size = GET_MODE_SIZE (mode);
^ permalink raw reply [flat|nested] 42+ messages in thread
* [3/8] Add narrow_bit_field_mem
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
2012-11-03 11:13 ` [1/8] Handle TRUNCATE in make_extraction Richard Sandiford
2012-11-03 11:14 ` [2/8] Add adjust_bitfield_address_size Richard Sandiford
@ 2012-11-03 11:16 ` Richard Sandiford
2012-11-10 16:02 ` Eric Botcazou
2012-11-03 11:21 ` [4/8] Add bit_field_mode_iterator Richard Sandiford
` (5 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:16 UTC (permalink / raw)
To: gcc-patches
This patch is the return of narrow_bit_field_mem, originally posted as
part of the preliminary patches. This time I've used a pointer rather
than a reference for the final parameter. (I agree consistency is good.)
The new version also takes the size of the field as a parameter and
handles BLKmode references.
Tested as described in the covering note. OK to install?
Richard
gcc/
* expmed.c (narrow_bit_field_mem): New function.
(store_bit_field_using_insv, store_bit_field_1, store_fixed_bit_field)
(extract_bit_field_1): Use it.
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c 2012-11-01 19:43:32.376371670 +0000
+++ gcc/expmed.c 2012-11-01 19:54:15.978370095 +0000
@@ -387,6 +387,34 @@ mode_for_extraction (enum extraction_pat
return data->operand[opno].mode;
}
+/* Adjust bitfield memory MEM so that it points to the first unit of mode
+ MODE that contains a bitfield of size BITSIZE at bit position BITNUM.
+ If MODE is BLKmode, return a reference to every byte in the bitfield.
+ Set *NEW_BITNUM to the bit position of the field within the new memory. */
+
+static rtx
+narrow_bit_field_mem (rtx mem, enum machine_mode mode,
+ unsigned HOST_WIDE_INT bitsize,
+ unsigned HOST_WIDE_INT bitnum,
+ unsigned HOST_WIDE_INT *new_bitnum)
+{
+ if (mode == BLKmode)
+ {
+ *new_bitnum = bitnum % BITS_PER_UNIT;
+ HOST_WIDE_INT offset = bitnum / BITS_PER_UNIT;
+ HOST_WIDE_INT size = ((*new_bitnum + bitsize + BITS_PER_UNIT - 1)
+ / BITS_PER_UNIT);
+ return adjust_bitfield_address_size (mem, mode, offset, size);
+ }
+ else
+ {
+ unsigned int unit = GET_MODE_BITSIZE (mode);
+ *new_bitnum = bitnum % unit;
+ HOST_WIDE_INT offset = (bitnum - *new_bitnum) / BITS_PER_UNIT;
+ return adjust_bitfield_address (mem, mode, offset);
+ }
+}
+
/* Return true if a bitfield of size BITSIZE at bit number BITNUM within
a structure of mode STRUCT_MODE represents a lowpart subreg. The subreg
offset is then BITNUM / BITS_PER_UNIT. */
@@ -424,11 +452,8 @@ store_bit_field_using_insv (rtx op0, uns
return false;
if (MEM_P (xop0))
- {
- /* Get a reference to the first byte of the field. */
- xop0 = adjust_bitfield_address (xop0, byte_mode, bitnum / BITS_PER_UNIT);
- bitnum %= BITS_PER_UNIT;
- }
+ /* Get a reference to the first byte of the field. */
+ xop0 = narrow_bit_field_mem (xop0, byte_mode, bitsize, bitnum, &bitnum);
else
{
/* Convert from counting within OP0 to counting in OP_MODE. */
@@ -831,18 +856,15 @@ store_bit_field_1 (rtx str_rtx, unsigned
&& GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
{
rtx last, tempreg, xop0;
- unsigned int unit;
- unsigned HOST_WIDE_INT offset, bitpos;
+ unsigned HOST_WIDE_INT bitpos;
last = get_last_insn ();
/* Adjust address to point to the containing unit of
that mode. Compute the offset as a multiple of this unit,
counting in bytes. */
- unit = GET_MODE_BITSIZE (bestmode);
- offset = (bitnum / unit) * GET_MODE_SIZE (bestmode);
- bitpos = bitnum % unit;
- xop0 = adjust_bitfield_address (op0, bestmode, offset);
+ xop0 = narrow_bit_field_mem (op0, bestmode, bitsize, bitnum,
+ &bitpos);
/* Fetch that unit, store the bitfield in it, then store
the unit. */
@@ -975,9 +997,7 @@ store_fixed_bit_field (rtx op0, unsigned
return;
}
- HOST_WIDE_INT bit_offset = bitnum - bitnum % GET_MODE_BITSIZE (mode);
- op0 = adjust_bitfield_address (op0, mode, bit_offset / BITS_PER_UNIT);
- bitnum -= bit_offset;
+ op0 = narrow_bit_field_mem (op0, mode, bitsize, bitnum, &bitnum);
}
mode = GET_MODE (op0);
@@ -1246,11 +1266,8 @@ extract_bit_field_using_extv (rtx op0, u
return NULL_RTX;
if (MEM_P (op0))
- {
- /* Get a reference to the first byte of the field. */
- op0 = adjust_bitfield_address (op0, byte_mode, bitnum / BITS_PER_UNIT);
- bitnum %= BITS_PER_UNIT;
- }
+ /* Get a reference to the first byte of the field. */
+ op0 = narrow_bit_field_mem (op0, byte_mode, bitsize, bitnum, &bitnum);
else
{
/* Convert from counting within OP0 to counting in EXT_MODE. */
@@ -1640,23 +1657,20 @@ extract_bit_field_1 (rtx str_rtx, unsign
&& !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
&& GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
{
- unsigned HOST_WIDE_INT offset, bitpos;
-
- /* Compute the offset as a multiple of this unit,
- counting in bytes. */
- unsigned int unit = GET_MODE_BITSIZE (bestmode);
- offset = (bitnum / unit) * GET_MODE_SIZE (bestmode);
- bitpos = bitnum % unit;
-
- /* Make sure the register is big enough for the whole field. */
- if (bitpos + bitsize <= unit)
+ unsigned HOST_WIDE_INT bitpos;
+ rtx xop0 = narrow_bit_field_mem (op0, bestmode, bitsize, bitnum,
+ &bitpos);
+
+ /* Make sure the register is big enough for the whole field.
+ (It might not be if bestmode == GET_MODE (op0) and the input
+ code was invalid.) */
+ if (bitpos + bitsize <= GET_MODE_BITSIZE (bestmode))
{
- rtx last, result, xop0;
+ rtx last, result;
last = get_last_insn ();
/* Fetch it to a register in that size. */
- xop0 = adjust_bitfield_address (op0, bestmode, offset);
xop0 = force_reg (bestmode, xop0);
result = extract_bit_field_1 (xop0, bitsize, bitpos,
unsignedp, packedp, target,
^ permalink raw reply [flat|nested] 42+ messages in thread
* [4/8] Add bit_field_mode_iterator
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (2 preceding siblings ...)
2012-11-03 11:16 ` [3/8] Add narrow_bit_field_mem Richard Sandiford
@ 2012-11-03 11:21 ` Richard Sandiford
2012-11-13 12:44 ` Eric Botcazou
2012-11-03 11:27 ` [5/8] Tweak bitfield alignment handling Richard Sandiford
` (4 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:21 UTC (permalink / raw)
To: gcc-patches
get_best_mode has various checks to decide what counts as an acceptable
bitfield mode. It actually has two copies of them, with slightly different
alignment checks:
MIN (unit, BIGGEST_ALIGNMENT) > align
vs.
unit <= MIN (align, BIGGEST_ALIGNMENT)
The second looks more correct, since we can't necessarily guarantee
larger alignments than BIGGEST_ALIGNMENT in all cases.
This patch adds a new iterator class that can be used to walk through
the modes, and rewrites get_best_mode to use it. I kept the existing
checks with two changes:
- bitregion_start is now tested independently of bitregion_end
- MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined
It shouldn't make any difference in practice, but both changes felt
more in keeping with the documentation of bitregion_start and
MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
test to be separate from bitregion_start.
The behaviour of the Sequent i386 compiler probably isn't the
issue it once was, but that's also dealt with in the next patch.
Tested as described in the covering note. OK to install?
Richard
gcc/
* machmode.h (bit_field_mode_iterator): New class.
(get_best_mode): Change final parameter to bool.
* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
(bit_field_mode_iterator::next_mode): New functions, split out from...
(get_best_mode): ...here. Change final parameter to bool.
Use bit_field_mode_iterator.
Index: gcc/machmode.h
===================================================================
--- gcc/machmode.h 2012-11-02 22:43:15.633373159 +0000
+++ gcc/machmode.h 2012-11-02 22:47:03.291372600 +0000
@@ -259,13 +259,36 @@ extern enum machine_mode int_mode_for_mo
extern enum machine_mode mode_for_vector (enum machine_mode, unsigned);
+/* A class for iterating through possible bitfield modes. */
+class bit_field_mode_iterator
+{
+public:
+ bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT,
+ HOST_WIDE_INT, HOST_WIDE_INT,
+ unsigned int, bool);
+ bool next_mode (enum machine_mode *);
+ bool prefer_smaller_modes ();
+
+private:
+ enum machine_mode mode_;
+ /* We use signed values here because the bit position can be negative
+ for invalid input such as gcc.dg/pr48335-8.c. */
+ HOST_WIDE_INT bitsize_;
+ HOST_WIDE_INT bitpos_;
+ HOST_WIDE_INT bitregion_start_;
+ HOST_WIDE_INT bitregion_end_;
+ unsigned int align_;
+ bool volatilep_;
+ int count_;
+};
+
/* Find the best mode to use to access a bit field. */
extern enum machine_mode get_best_mode (int, int,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned int,
- enum machine_mode, int);
+ enum machine_mode, bool);
/* Determine alignment, 1<=result<=BIGGEST_ALIGNMENT. */
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-02 22:43:15.633373159 +0000
+++ gcc/stor-layout.c 2012-11-02 22:47:11.367372581 +0000
@@ -2624,6 +2624,98 @@ fixup_unsigned_type (tree type)
layout_type (type);
}
\f
+/* Construct an iterator for a bitfield that spans BITSIZE bits,
+ starting at BITPOS.
+
+ BITREGION_START is the bit position of the first bit in this
+ sequence of bit fields. BITREGION_END is the last bit in this
+ sequence. If these two fields are non-zero, we should restrict the
+ memory access to a maximum sized chunk of
+ BITREGION_END - BITREGION_START + 1. Otherwise, we are allowed to touch
+ any adjacent non bit-fields.
+
+ ALIGN is the alignment of the underlying object in bits.
+ VOLATILEP says whether the bitfield is volatile. */
+
+bit_field_mode_iterator
+::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
+ HOST_WIDE_INT bitregion_start,
+ HOST_WIDE_INT bitregion_end,
+ unsigned int align, bool volatilep)
+: mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize),
+ bitpos_ (bitpos), bitregion_start_ (bitregion_start),
+ bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
+ volatilep_ (volatilep), count_ (0)
+{
+ if (bitregion_end_)
+ bitregion_end_ += 1;
+}
+
+/* Calls to this function return successively larger modes that can be used
+ to represent the bitfield. Return true if another bitfield mode is
+ available, storing it in *OUT_MODE if so. */
+
+bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)
+{
+ for (; mode_ != VOIDmode; mode_ = GET_MODE_WIDER_MODE (mode_))
+ {
+ unsigned int unit = GET_MODE_BITSIZE (mode_);
+
+ /* Skip modes that don't have full precision. */
+ if (unit != GET_MODE_PRECISION (mode_))
+ continue;
+
+ /* Skip modes that are too small. */
+ if ((bitpos_ % unit) + bitsize_ > unit)
+ continue;
+
+ /* Stop if the mode is too wide to handle efficiently. */
+ if (unit > MAX_FIXED_MODE_SIZE)
+ break;
+
+ /* Don't deliver more than one multiword mode; the smallest one
+ should be used. */
+ if (count_ > 0 && unit > BITS_PER_WORD)
+ break;
+
+ /* Stop if the mode is wider than the alignment of the containing
+ object.
+
+ It is tempting to omit the following line unless STRICT_ALIGNMENT
+ is true. But that is incorrect, since if the bitfield uses part
+ of 3 bytes and we use a 4-byte mode, we could get a spurious segv
+ if the extra 4th byte is past the end of memory.
+ (Though at least one Unix compiler ignores this problem:
+ that on the Sequent 386 machine. */
+ if (unit > align_)
+ break;
+
+ /* Stop if the mode goes outside the bitregion. */
+ HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
+ if (bitregion_start_ && start < bitregion_start_)
+ break;
+ if (bitregion_end_ && start + unit > bitregion_end_)
+ break;
+
+ *out_mode = mode_;
+ mode_ = GET_MODE_WIDER_MODE (mode_);
+ count_++;
+ return true;
+ }
+ return false;
+}
+
+/* Return true if smaller modes are generally preferred for this kind
+ of bitfield. */
+
+bool
+bit_field_mode_iterator::prefer_smaller_modes ()
+{
+ return (volatilep_
+ ? targetm.narrow_volatile_bitfield ()
+ : !SLOW_BYTE_ACCESS);
+}
+
/* Find the best machine mode to use when referencing a bit field of length
BITSIZE bits starting at BITPOS.
@@ -2655,69 +2747,21 @@ get_best_mode (int bitsize, int bitpos,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
unsigned int align,
- enum machine_mode largest_mode, int volatilep)
+ enum machine_mode largest_mode, bool volatilep)
{
+ bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start,
+ bitregion_end, align, volatilep);
+ enum machine_mode widest_mode = VOIDmode;
enum machine_mode mode;
- unsigned int unit = 0;
- unsigned HOST_WIDE_INT maxbits;
-
- /* If unset, no restriction. */
- if (!bitregion_end)
- maxbits = MAX_FIXED_MODE_SIZE;
- else
- maxbits = bitregion_end - bitregion_start + 1;
-
- /* Find the narrowest integer mode that contains the bit field. */
- for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
- mode = GET_MODE_WIDER_MODE (mode))
+ while (iter.next_mode (&mode)
+ && (largest_mode == VOIDmode
+ || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
{
- unit = GET_MODE_BITSIZE (mode);
- if (unit == GET_MODE_PRECISION (mode)
- && (bitpos % unit) + bitsize <= unit)
+ widest_mode = mode;
+ if (iter.prefer_smaller_modes ())
break;
}
-
- if (mode == VOIDmode
- /* It is tempting to omit the following line
- if STRICT_ALIGNMENT is true.
- But that is incorrect, since if the bitfield uses part of 3 bytes
- and we use a 4-byte mode, we could get a spurious segv
- if the extra 4th byte is past the end of memory.
- (Though at least one Unix compiler ignores this problem:
- that on the Sequent 386 machine. */
- || MIN (unit, BIGGEST_ALIGNMENT) > align
- || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE (largest_mode))
- || unit > maxbits
- || (bitregion_end
- && bitpos - (bitpos % unit) + unit > bitregion_end + 1))
- return VOIDmode;
-
- if ((SLOW_BYTE_ACCESS && ! volatilep)
- || (volatilep && !targetm.narrow_volatile_bitfield ()))
- {
- enum machine_mode wide_mode = VOIDmode, tmode;
-
- for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT); tmode != VOIDmode;
- tmode = GET_MODE_WIDER_MODE (tmode))
- {
- unit = GET_MODE_BITSIZE (tmode);
- if (unit == GET_MODE_PRECISION (tmode)
- && bitpos / unit == (bitpos + bitsize - 1) / unit
- && unit <= BITS_PER_WORD
- && unit <= MIN (align, BIGGEST_ALIGNMENT)
- && unit <= maxbits
- && (largest_mode == VOIDmode
- || unit <= GET_MODE_BITSIZE (largest_mode))
- && (bitregion_end == 0
- || bitpos - (bitpos % unit) + unit <= bitregion_end + 1))
- wide_mode = tmode;
- }
-
- if (wide_mode != VOIDmode)
- return wide_mode;
- }
-
- return mode;
+ return widest_mode;
}
/* Gets minimal and maximal values for MODE (signed or unsigned depending on
^ permalink raw reply [flat|nested] 42+ messages in thread
* [5/8] Tweak bitfield alignment handling
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (3 preceding siblings ...)
2012-11-03 11:21 ` [4/8] Add bit_field_mode_iterator Richard Sandiford
@ 2012-11-03 11:27 ` Richard Sandiford
2012-11-13 13:52 ` Eric Botcazou
2012-11-03 11:28 ` [6/8] Add strict volatile handling to bit_field_mode_iterator Richard Sandiford
` (3 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:27 UTC (permalink / raw)
To: gcc-patches
This patch replaces:
/* Stop if the mode is wider than the alignment of the containing
object.
It is tempting to omit the following line unless STRICT_ALIGNMENT
is true. But that is incorrect, since if the bitfield uses part
of 3 bytes and we use a 4-byte mode, we could get a spurious segv
if the extra 4th byte is past the end of memory.
(Though at least one Unix compiler ignores this problem:
that on the Sequent 386 machine. */
if (unit > align_)
break;
with two checks: one for whether the final byte of the mode is known
to be mapped, and one for whether the bitfield is sufficiently aligned.
Later patches in the series rely on this in order not to pessimise
memory handling.
However, as described in the patch, I found that extending this
behaviour to get_best_mode affected code generation for x86_64-linux-gnu
and powerpc64-linux-gnu, and led to a failure in bb-slp-5.c on both.
I therefore chickened out and added the original check back to
get_best_mode.
I'm certainly interested in any feedback on the comment, but at the
same time I'd like this series to be a no-op on targets that keep
the traditional .md patterns. Any change to get_best_mode is probably
best done as a separate patch.
Tested as described in the covering note. OK to install?
Richard
gcc/
* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
Set up a default value of bitregion_end_.
(bit_field_mode_iterator::next_mode): Always apply bitregion_end_
check. Include SLOW_UNALIGNED_ACCESS in the alignment check.
(get_best_mode): Ignore modes that are wider than the alignment.
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-03 10:38:49.662357739 +0000
+++ gcc/stor-layout.c 2012-11-03 11:25:55.133350825 +0000
@@ -2649,6 +2649,13 @@ fixup_unsigned_type (tree type)
{
if (bitregion_end_)
bitregion_end_ += 1;
+ else
+ {
+ /* We can assume that any aligned chunk of ALIGN_ bits that overlaps
+ the bitfield is mapped and won't trap. */
+ bitregion_end_ = bitpos + bitsize + align_ - 1;
+ bitregion_end_ -= bitregion_end_ % align_;
+ }
}
/* Calls to this function return successively larger modes that can be used
@@ -2678,23 +2685,15 @@ bool bit_field_mode_iterator::next_mode
if (count_ > 0 && unit > BITS_PER_WORD)
break;
- /* Stop if the mode is wider than the alignment of the containing
- object.
-
- It is tempting to omit the following line unless STRICT_ALIGNMENT
- is true. But that is incorrect, since if the bitfield uses part
- of 3 bytes and we use a 4-byte mode, we could get a spurious segv
- if the extra 4th byte is past the end of memory.
- (Though at least one Unix compiler ignores this problem:
- that on the Sequent 386 machine. */
- if (unit > align_)
- break;
-
/* Stop if the mode goes outside the bitregion. */
HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
if (bitregion_start_ && start < bitregion_start_)
break;
- if (bitregion_end_ && start + unit > bitregion_end_)
+ if (start + unit > bitregion_end_)
+ break;
+
+ /* Stop if the mode requires too much alignment. */
+ if (unit > align_ && SLOW_UNALIGNED_ACCESS (mode_, align_))
break;
*out_mode = mode_;
@@ -2754,6 +2753,62 @@ get_best_mode (int bitsize, int bitpos,
enum machine_mode widest_mode = VOIDmode;
enum machine_mode mode;
while (iter.next_mode (&mode)
+ /* ??? For compatiblity, reject modes that are wider than the
+ alignment. This has both advantages and disadvantages.
+ Removing this check means that something like:
+
+ struct s { unsigned int x; unsigned int y; };
+ int f (struct s *s) { return s->x == 0 && s->y == 0; }
+
+ can be implemented using a single load and compare on
+ 64-bit machines that have no alignment restrictions.
+ For example, on powerpc64-linux-gnu, we would generate:
+
+ ld 3,0(3)
+ cntlzd 3,3
+ srdi 3,3,6
+ blr
+
+ rather than:
+
+ lwz 9,0(3)
+ cmpwi 7,9,0
+ bne 7,.L3
+ lwz 3,4(3)
+ cntlzw 3,3
+ srwi 3,3,5
+ extsw 3,3
+ blr
+ .p2align 4,,15
+ .L3:
+ li 3,0
+ blr
+
+ However, accessing more than one field can make life harder
+ for the gimple optimizers. For example, gcc.dg/vect/bb-slp-5.c
+ has a series of unsigned short copies followed by a series of
+ unsigned short comparisons. With this check, both the copies
+ and comparisons remain 16-bit accesses and FRE is able
+ to eliminate the latter. Without the check, the comparisons
+ can be done using 2 64-bit operations, which FRE isn't able
+ to handle in the same way.
+
+ Either way, it would probably be worth disabling this check
+ during expand. One particular example where removing the
+ check would help is the get_best_mode call in store_bit_field.
+ If we are given a memory bitregion of 128 bits that is aligned
+ to a 64-bit boundary, and the bitfield we want to modify is
+ in the second half of the bitregion, this check causes
+ store_bitfield to turn the memory into a 64-bit reference
+ to the _first_ half of the region. We later use
+ adjust_bitfield_address to get a reference to the correct half,
+ but doing so looks to adjust_bitfield_address as though we are
+ moving past the end of the original object, so it drops the
+ associated MEM_EXPR and MEM_OFFSET. Removing the check
+ causes store_bit_field to keep a 128-bit memory reference,
+ so that the final bitfield reference still has a MEM_EXPR
+ and MEM_OFFSET. */
+ && GET_MODE_BITSIZE (mode) <= align
&& (largest_mode == VOIDmode
|| GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
{
^ permalink raw reply [flat|nested] 42+ messages in thread
* [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (4 preceding siblings ...)
2012-11-03 11:27 ` [5/8] Tweak bitfield alignment handling Richard Sandiford
@ 2012-11-03 11:28 ` Richard Sandiford
2012-11-13 13:57 ` Eric Botcazou
2012-11-03 11:39 ` [7/8] Replace mode_for_extraction with new interface Richard Sandiford
` (2 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:28 UTC (permalink / raw)
To: gcc-patches
This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
into account, in cases where the size of the underlying object is known.
This is used in the next patch.
Tested as described in the covering note. OK to install?
Richard
gcc/
* machmode.h (bit_field_mode_iterator::bit_field_mode_iterator):
Add an object_bitsize parameter.
(bit_field_mode_iterator): Add object_bitsize_.
* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
Add an object_bitsize parameter. Initialize object_bitsize_.
(bit_field_mode_iterator::next_mode): For strict volatile fields,
reject modes that aren't the same width as the underlying object.
(get_best_mode): Update bit_field_mode_iterator constructor.
Index: gcc/machmode.h
===================================================================
--- gcc/machmode.h 2012-11-02 22:47:03.291372600 +0000
+++ gcc/machmode.h 2012-11-02 23:14:00.225368645 +0000
@@ -265,7 +265,7 @@ extern enum machine_mode mode_for_vector
public:
bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT,
HOST_WIDE_INT, HOST_WIDE_INT,
- unsigned int, bool);
+ unsigned HOST_WIDE_INT, unsigned int, bool);
bool next_mode (enum machine_mode *);
bool prefer_smaller_modes ();
@@ -277,6 +277,7 @@ extern enum machine_mode mode_for_vector
HOST_WIDE_INT bitpos_;
HOST_WIDE_INT bitregion_start_;
HOST_WIDE_INT bitregion_end_;
+ unsigned HOST_WIDE_INT object_bitsize_;
unsigned int align_;
bool volatilep_;
int count_;
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-02 23:10:07.316369215 +0000
+++ gcc/stor-layout.c 2012-11-02 23:14:00.224368645 +0000
@@ -2634,6 +2634,8 @@ fixup_unsigned_type (tree type)
BITREGION_END - BITREGION_START + 1. Otherwise, we are allowed to touch
any adjacent non bit-fields.
+ OBJECT_BITSIZE is the number of bits that should be used to access
+ a strict volatile bitfield, or 0 if not known.
ALIGN is the alignment of the underlying object in bits.
VOLATILEP says whether the bitfield is volatile. */
@@ -2641,11 +2643,12 @@ fixup_unsigned_type (tree type)
::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
HOST_WIDE_INT bitregion_start,
HOST_WIDE_INT bitregion_end,
+ unsigned HOST_WIDE_INT object_bitsize,
unsigned int align, bool volatilep)
: mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize),
bitpos_ (bitpos), bitregion_start_ (bitregion_start),
- bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
- volatilep_ (volatilep), count_ (0)
+ bitregion_end_ (bitregion_end), object_bitsize_ (object_bitsize),
+ align_ (MIN (align, BIGGEST_ALIGNMENT)), volatilep_ (volatilep), count_ (0)
{
if (bitregion_end_)
bitregion_end_ += 1;
@@ -2676,6 +2679,14 @@ bool bit_field_mode_iterator::next_mode
if ((bitpos_ % unit) + bitsize_ > unit)
continue;
+ /* If the field is strict volatile, skip modes that are not the same
+ as the object size. */
+ if (object_bitsize_
+ && unit != object_bitsize_
+ && volatilep_
+ && flag_strict_volatile_bitfields > 0)
+ continue;
+
/* Stop if the mode is too wide to handle efficiently. */
if (unit > MAX_FIXED_MODE_SIZE)
break;
@@ -2749,7 +2760,7 @@ get_best_mode (int bitsize, int bitpos,
enum machine_mode largest_mode, bool volatilep)
{
bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start,
- bitregion_end, align, volatilep);
+ bitregion_end, 0, align, volatilep);
enum machine_mode widest_mode = VOIDmode;
enum machine_mode mode;
while (iter.next_mode (&mode)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [7/8] Replace mode_for_extraction with new interface
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (5 preceding siblings ...)
2012-11-03 11:28 ` [6/8] Add strict volatile handling to bit_field_mode_iterator Richard Sandiford
@ 2012-11-03 11:39 ` Richard Sandiford
2012-11-03 11:41 ` [8/8] Add new optabs and use them for MIPS Richard Sandiford
2012-11-27 17:11 ` [0/8] Add optabs alternatives for insv, extv and extzv Ramana Radhakrishnan
8 siblings, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:39 UTC (permalink / raw)
To: gcc-patches
This patch adds a new interface for asking about insertion and extraction
instructions, to replace mode_for_extraction. This first version just uses
the traditional patterns; the next patch actually adds the new optabs.
recog has some odd code to change the mode of a memory involved in a
sign_extract or zero_extract. It predates svn history, but as best
I can tell, its main purpose was to change the memory reference to
the traditional QImode one in cases where the memory has appeared
through register substitution. However, this isn't something that
we want for the new optabs, and I doubt many targets would welcome
it even for extv and extzv. Rather than keep mode_for_extraction
around for this one use, I thought it would be better to probe insn_data
directly, even though that's in some ways a reversion of Zack's original
clean-up patch.
Tested as described in the covering note. OK to install?
Richard
gcc/
* Makefile.in (recog.o): Add insn-codes.h.
* expr.h (extraction_pattern): Move to optabs.h.
(mode_for_extraction): Delete.
* optabs.h (extraction_insn): New structure.
(extraction_pattern): Moved from expr.h.
(get_best_reg_extraction_insn, get_best_mem_extraction_insn): Declare.
* optabs.c (HAVE_insv, CODE_FOR_insv, HAVE_extv, CODE_FOR_extv)
(HAVE_extzv, CODE_FOR_extzv): Provide defaults.
(extraction_type): New enum.
(get_traditional_extraction_insn, get_extraction_insn)
(get_best_reg_extraction_insn, get_best_mem_extraction_insn):
New functions.
* combine.c (make_extraction): Use get_best_reg_extraction_insn
instead of mode_for_extraction.
* expmed.c (HAVE_insv, CODE_FOR_insv, gen_insv, HAVE_extv)
(CODE_FOR_extv, gen_extv, HAVE_extzv, CODE_FOR_extzv, gen_extzv):
Remove fallback definitions.
(mode_for_extraction): Delete.
(adjust_bit_field_mem_for_reg): New function.
(store_bit_field_using_insv): Replace OP_MODE parameter with
an extraction_insn. Pass struct_mode to narrow_bit_field_mem.
(extract_bit_field_using_extv): Likewise EXT_MODE.
(store_bit_field_1): Use get_best_reg_extraction_insn and
get_best_mem_extraction_insn instead of mode_for_extraction.
Use adjust_bit_field_mem_for_reg when forcing memory to a
register and doing a register insertion. Update calls to
store_bit_field_using_insv.
(extract_bit_field_1): Likewise extractions and calls to
extract_bit_field_using_extv.
(store_Bit_field): When narrowing to a bitregion, don't use the
insv mode as a limit.
* recog.c: (HAVE_extv, CODE_FOR_extv, HAVE_extzv, CODE_FOR_extzv):
Provide defaults.
(simplify_while_replacing): Use insn_data instead of
mode_for_extraction.
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in 2012-11-03 09:21:20.685369114 +0000
+++ gcc/Makefile.in 2012-11-03 11:32:18.691349886 +0000
@@ -3334,7 +3334,7 @@ recog.o : recog.c $(CONFIG_H) $(SYSTEM_H
$(FUNCTION_H) $(BASIC_BLOCK_H) $(REGS_H) $(RECOG_H) $(EXPR_H) \
$(FLAGS_H) insn-config.h $(INSN_ATTR_H) reload.h \
addresses.h $(TM_P_H) $(TREE_PASS_H) hard-reg-set.h \
- $(DF_H) $(DBGCNT_H) $(TARGET_H) $(DIAGNOSTIC_CORE_H)
+ $(DF_H) $(DBGCNT_H) $(TARGET_H) $(DIAGNOSTIC_CORE_H) insn-codes.h
reg-stack.o : reg-stack.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
$(RTL_ERROR_H) $(TREE_H) $(RECOG_H) $(REGS_H) hard-reg-set.h $(FLAGS_H) \
insn-config.h reload.h $(FUNCTION_H) $(TM_P_H) $(GGC_H) \
Index: gcc/expr.h
===================================================================
--- gcc/expr.h 2012-11-03 10:38:48.827357741 +0000
+++ gcc/expr.h 2012-11-03 11:32:18.671349886 +0000
@@ -698,14 +698,6 @@ extern void probe_stack_range (HOST_WIDE
in its original home. This becomes invalid if any more code is emitted. */
extern rtx hard_libcall_value (enum machine_mode, rtx);
-/* Return the mode desired by operand N of a particular bitfield
- insert/extract insn, or MAX_MACHINE_MODE if no such insn is
- available. */
-
-enum extraction_pattern { EP_insv, EP_extv, EP_extzv };
-extern enum machine_mode
-mode_for_extraction (enum extraction_pattern, int);
-
extern void store_bit_field (rtx, unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
Index: gcc/optabs.h
===================================================================
--- gcc/optabs.h 2012-11-02 23:45:19.194364048 +0000
+++ gcc/optabs.h 2012-11-03 11:32:18.678349886 +0000
@@ -323,6 +323,38 @@ extern rtx optab_libfunc (optab optab, e
extern rtx convert_optab_libfunc (convert_optab optab, enum machine_mode mode1,
enum machine_mode mode2);
+/* Describes an instruction that inserts or extracts a bitfield. */
+struct extraction_insn
+{
+ /* The code of the instruction. */
+ enum insn_code icode;
+
+ /* The mode that the structure operand should have. This is byte_mode
+ when using the legacy insv, extv and extzv patterns to access memory. */
+ enum machine_mode struct_mode;
+
+ /* The mode of the field to be inserted or extracted, and by extension
+ the mode of the insertion or extraction itself. */
+ enum machine_mode field_mode;
+
+ /* The mode of the field's bit position. This is only important
+ when the position is variable rather than constant. */
+ enum machine_mode pos_mode;
+};
+
+/* Enumerates the possible extraction_insn operations. */
+enum extraction_pattern { EP_insv, EP_extv, EP_extzv };
+
+extern bool get_best_reg_extraction_insn (extraction_insn *,
+ enum extraction_pattern,
+ unsigned HOST_WIDE_INT,
+ enum machine_mode);
+
+extern bool get_best_mem_extraction_insn (extraction_insn *,
+ enum extraction_pattern,
+ HOST_WIDE_INT, HOST_WIDE_INT,
+ enum machine_mode);
+
extern bool insn_operand_matches (enum insn_code icode, unsigned int opno,
rtx operand);
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c 2012-11-03 09:21:20.644369114 +0000
+++ gcc/optabs.c 2012-11-03 11:35:43.109349387 +0000
@@ -8239,4 +8239,177 @@ expand_jump_insn (enum insn_code icode,
gcc_unreachable ();
}
+/* Reduce conditional compilation elsewhere. */
+#ifndef HAVE_insv
+#define HAVE_insv 0
+#define CODE_FOR_insv CODE_FOR_nothing
+#endif
+#ifndef HAVE_extv
+#define HAVE_extv 0
+#define CODE_FOR_extv CODE_FOR_nothing
+#endif
+#ifndef HAVE_extzv
+#define HAVE_extzv 0
+#define CODE_FOR_extzv CODE_FOR_nothing
+#endif
+
+/* Enumerates the possible types of structure operand to an
+ extraction_insn. */
+enum extraction_type { ET_unaligned_mem, ET_reg };
+
+/* Check whether insv, extv or extzv pattern ICODE can be used for an
+ insertion or extraction of type TYPE on a structure of mode MODE.
+ Return true if so and fill in *INSN accordingly. STRUCT_OP is the
+ operand number of the structure (the first sign_extract or zero_extract
+ operand) and FIELD_OP is the operand number of the field (the other
+ side of the set from the sign_extract or zero_extract). */
+
+static bool
+get_traditional_extraction_insn (extraction_insn *insn,
+ enum extraction_type type,
+ enum machine_mode mode,
+ enum insn_code icode,
+ int struct_op, int field_op)
+{
+ const struct insn_data_d *data = &insn_data[icode];
+
+ enum machine_mode struct_mode = data->operand[struct_op].mode;
+ if (struct_mode == VOIDmode)
+ struct_mode = word_mode;
+ if (mode != struct_mode)
+ return false;
+
+ enum machine_mode field_mode = data->operand[field_op].mode;
+ if (field_mode == VOIDmode)
+ field_mode = word_mode;
+
+ enum machine_mode pos_mode = data->operand[struct_op + 2].mode;
+ if (pos_mode == VOIDmode)
+ pos_mode = word_mode;
+
+ insn->icode = icode;
+ insn->field_mode = field_mode;
+ insn->struct_mode = (type == ET_unaligned_mem ? byte_mode : struct_mode);
+ insn->pos_mode = pos_mode;
+ return true;
+}
+
+/* Return true if an instruction exists to perform an insertion or
+ extraction (PATTERN says which) of type TYPE in mode MODE.
+ Describe the instruction in *INSN if so. */
+
+static bool
+get_extraction_insn (extraction_insn *insn,
+ enum extraction_pattern pattern,
+ enum extraction_type type,
+ enum machine_mode mode)
+{
+ switch (pattern)
+ {
+ case EP_insv:
+ if (HAVE_insv
+ && get_traditional_extraction_insn (insn, type, mode,
+ CODE_FOR_insv, 0, 3))
+ return true;
+ return false;
+
+ case EP_extv:
+ if (HAVE_extv
+ && get_traditional_extraction_insn (insn, type, mode,
+ CODE_FOR_extv, 1, 0))
+ return true;
+ return false;
+
+ case EP_extzv:
+ if (HAVE_extzv
+ && get_traditional_extraction_insn (insn, type, mode,
+ CODE_FOR_extzv, 1, 0))
+ return true;
+ return false;
+
+ default:
+ gcc_unreachable ();
+ }
+}
+
+/* Return true if an instruction exists to access a field of mode
+ FIELDMODE in a structure that has STRUCT_BITS significant bits.
+ Describe the "best" such instruction in *INSN if so. PATTERN and
+ TYPE describe the type of insertion or extraction we want to perform.
+
+ For an insertion, the number of significant structure bits includes
+ all bits of the target. For an extraction, it need only include the
+ most significant bit of the field. Larger widths are acceptable
+ in both cases. */
+
+static bool
+get_best_extraction_insn (extraction_insn *insn,
+ enum extraction_pattern pattern,
+ enum extraction_type type,
+ unsigned HOST_WIDE_INT struct_bits,
+ enum machine_mode field_mode)
+{
+ enum machine_mode mode = smallest_mode_for_size (struct_bits, MODE_INT);
+ while (mode != VOIDmode)
+ {
+ if (get_extraction_insn (insn, pattern, type, mode))
+ {
+ while (mode != VOIDmode
+ && GET_MODE_SIZE (mode) <= GET_MODE_SIZE (field_mode)
+ && !TRULY_NOOP_TRUNCATION_MODES_P (insn->field_mode,
+ field_mode))
+ {
+ get_extraction_insn (insn, pattern, type, mode);
+ mode = GET_MODE_WIDER_MODE (mode);
+ }
+ return true;
+ }
+ mode = GET_MODE_WIDER_MODE (mode);
+ }
+ return false;
+}
+
+/* Return true if an instruction exists to access a field of mode
+ FIELDMODE in a register structure that has STRUCT_BITS significant bits.
+ Describe the "best" such instruction in *INSN if so. PATTERN describes
+ the type of insertion or extraction we want to perform.
+
+ For an insertion, the number of significant structure bits includes
+ all bits of the target. For an extraction, it need only include the
+ most significant bit of the field. Larger widths are acceptable
+ in both cases. */
+
+bool
+get_best_reg_extraction_insn (extraction_insn *insn,
+ enum extraction_pattern pattern,
+ unsigned HOST_WIDE_INT struct_bits,
+ enum machine_mode field_mode)
+{
+ return get_best_extraction_insn (insn, pattern, ET_reg, struct_bits,
+ field_mode);
+}
+
+/* Return true if an instruction exists to access a field of BITSIZE
+ bits starting BITNUM bits into a memory structure. Describe the
+ "best" such instruction in *INSN if so. PATTERN describes the type
+ of insertion or extraction we want to perform and FIELDMODE is the
+ natural mode of the extracted field.
+
+ The instructions considered here only access bytes that overlap
+ the bitfield; they do not touch any surrounding bytes. */
+
+bool
+get_best_mem_extraction_insn (extraction_insn *insn,
+ enum extraction_pattern pattern,
+ HOST_WIDE_INT bitsize, HOST_WIDE_INT bitnum,
+ enum machine_mode field_mode)
+{
+ unsigned HOST_WIDE_INT struct_bits = (bitnum % BITS_PER_UNIT
+ + bitsize
+ + BITS_PER_UNIT - 1);
+ struct_bits -= struct_bits % BITS_PER_UNIT;
+ return get_best_extraction_insn (insn, pattern, ET_unaligned_mem,
+ struct_bits, field_mode);
+}
+
#include "gt-optabs.h"
Index: gcc/combine.c
===================================================================
--- gcc/combine.c 2012-11-03 10:38:48.259357742 +0000
+++ gcc/combine.c 2012-11-03 11:36:49.903349223 +0000
@@ -7182,29 +7182,24 @@ make_extraction (enum machine_mode mode,
|| (pos_rtx != 0 && len != 1)))
return 0;
- /* Get the mode to use should INNER not be a MEM, the mode for the position,
- and the mode for the result. */
- if (in_dest && mode_for_extraction (EP_insv, -1) != MAX_MACHINE_MODE)
- {
- wanted_inner_reg_mode = mode_for_extraction (EP_insv, 0);
- pos_mode = mode_for_extraction (EP_insv, 2);
- extraction_mode = mode_for_extraction (EP_insv, 3);
- }
-
- if (! in_dest && unsignedp
- && mode_for_extraction (EP_extzv, -1) != MAX_MACHINE_MODE)
- {
- wanted_inner_reg_mode = mode_for_extraction (EP_extzv, 1);
- pos_mode = mode_for_extraction (EP_extzv, 3);
- extraction_mode = mode_for_extraction (EP_extzv, 0);
- }
+ enum extraction_pattern pattern = (in_dest ? EP_insv
+ : unsignedp ? EP_extzv : EP_extv);
- if (! in_dest && ! unsignedp
- && mode_for_extraction (EP_extv, -1) != MAX_MACHINE_MODE)
+ /* If INNER is not from memory, we want it to have the mode of a register
+ extraction pattern's structure operand, or word_mode if there is no
+ such pattern. The same applies to extraction_mode and pos_mode
+ and their respective operands.
+
+ For memory, assume that the desired extraction_mode and pos_mode
+ are the same as for a register operation, since at present we don't
+ have named patterns for aligned memory structures. */
+ struct extraction_insn insn;
+ if (get_best_reg_extraction_insn (&insn, pattern,
+ GET_MODE_BITSIZE (inner_mode), mode))
{
- wanted_inner_reg_mode = mode_for_extraction (EP_extv, 1);
- pos_mode = mode_for_extraction (EP_extv, 3);
- extraction_mode = mode_for_extraction (EP_extv, 0);
+ wanted_inner_reg_mode = insn.struct_mode;
+ pos_mode = insn.pos_mode;
+ extraction_mode = insn.field_mode;
}
/* Never narrow an object, since that might not be safe. */
@@ -7213,9 +7208,6 @@ make_extraction (enum machine_mode mode,
&& GET_MODE_SIZE (extraction_mode) < GET_MODE_SIZE (mode))
extraction_mode = mode;
- /* If this is not from memory, the desired mode is the preferred mode
- for an extraction pattern's first input operand, or word_mode if there
- is none. */
if (!MEM_P (inner))
wanted_inner_mode = wanted_inner_reg_mode;
else
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c 2012-11-03 10:38:49.258357740 +0000
+++ gcc/expmed.c 2012-11-03 11:32:18.669349886 +0000
@@ -69,23 +69,6 @@ static rtx expand_sdiv_pow2 (enum machin
/* Test whether a value is zero of a power of two. */
#define EXACT_POWER_OF_2_OR_ZERO_P(x) (((x) & ((x) - 1)) == 0)
-/* Reduce conditional compilation elsewhere. */
-#ifndef HAVE_insv
-#define HAVE_insv 0
-#define CODE_FOR_insv CODE_FOR_nothing
-#define gen_insv(a,b,c,d) NULL_RTX
-#endif
-#ifndef HAVE_extv
-#define HAVE_extv 0
-#define CODE_FOR_extv CODE_FOR_nothing
-#define gen_extv(a,b,c,d) NULL_RTX
-#endif
-#ifndef HAVE_extzv
-#define HAVE_extzv 0
-#define CODE_FOR_extzv CODE_FOR_nothing
-#define gen_extzv(a,b,c,d) NULL_RTX
-#endif
-
struct init_expmed_rtl
{
struct rtx_def reg; rtunion reg_fld[2];
@@ -338,55 +321,6 @@ negate_rtx (enum machine_mode mode, rtx
return result;
}
-/* Report on the availability of insv/extv/extzv and the desired mode
- of each of their operands. Returns MAX_MACHINE_MODE if HAVE_foo
- is false; else the mode of the specified operand. If OPNO is -1,
- all the caller cares about is whether the insn is available. */
-enum machine_mode
-mode_for_extraction (enum extraction_pattern pattern, int opno)
-{
- const struct insn_data_d *data;
-
- switch (pattern)
- {
- case EP_insv:
- if (HAVE_insv)
- {
- data = &insn_data[CODE_FOR_insv];
- break;
- }
- return MAX_MACHINE_MODE;
-
- case EP_extv:
- if (HAVE_extv)
- {
- data = &insn_data[CODE_FOR_extv];
- break;
- }
- return MAX_MACHINE_MODE;
-
- case EP_extzv:
- if (HAVE_extzv)
- {
- data = &insn_data[CODE_FOR_extzv];
- break;
- }
- return MAX_MACHINE_MODE;
-
- default:
- gcc_unreachable ();
- }
-
- if (opno == -1)
- return VOIDmode;
-
- /* Everyone who uses this function used to follow it with
- if (result == VOIDmode) result = word_mode; */
- if (data->operand[opno].mode == VOIDmode)
- return word_mode;
- return data->operand[opno].mode;
-}
-
/* Adjust bitfield memory MEM so that it points to the first unit of mode
MODE that contains a bitfield of size BITSIZE at bit position BITNUM.
If MODE is BLKmode, return a reference to every byte in the bitfield.
@@ -415,6 +349,60 @@ narrow_bit_field_mem (rtx mem, enum mach
}
}
+/* The caller wants to perform insertion of extraction PATTERN on a
+ bitfield of size BITSIZE at BITNUM bits into memory operand OP0.
+ BITREGION_START and BITREGION_END are as for store_bit_field
+ and FIELDMODE is the natural mode of the field.
+
+ Search for a mode that is compatible with the memory access
+ restrictions and (where applicable) with a register insertion or
+ extraction. Return the new memory on success, storing the adjusted
+ bit position in *NEW_BITNUM. Return null otherwise. */
+
+static rtx
+adjust_bit_field_mem_for_reg (enum extraction_pattern pattern,
+ rtx op0, HOST_WIDE_INT bitsize,
+ HOST_WIDE_INT bitnum,
+ unsigned HOST_WIDE_INT bitregion_start,
+ unsigned HOST_WIDE_INT bitregion_end,
+ enum machine_mode fieldmode,
+ unsigned HOST_WIDE_INT *new_bitnum)
+{
+ unsigned HOST_WIDE_INT op0_size = (MEM_SIZE_KNOWN_P (op0)
+ ? MEM_SIZE (op0) * BITS_PER_UNIT
+ : 0);
+ bit_field_mode_iterator iter (bitsize, bitnum, bitregion_start,
+ bitregion_end, op0_size, MEM_ALIGN (op0),
+ MEM_VOLATILE_P (op0));
+ enum machine_mode best_mode;
+ if (iter.next_mode (&best_mode))
+ {
+ /* We can use a memory in BEST_MODE. See whether this is true for
+ any wider modes. All other things being equal, we prefer to
+ use the widest mode possible because it tends to expose more
+ CSE opportunities. */
+ if (!iter.prefer_smaller_modes ())
+ {
+ /* Limit the search to the mode required by the corresponding
+ register insertion or extraction instruction, if any. */
+ enum machine_mode limit_mode = word_mode;
+ extraction_insn insn;
+ if (get_best_reg_extraction_insn (&insn, pattern,
+ GET_MODE_BITSIZE (best_mode),
+ fieldmode))
+ limit_mode = insn.field_mode;
+
+ enum machine_mode wider_mode;
+ while (iter.next_mode (&wider_mode)
+ && GET_MODE_SIZE (wider_mode) <= GET_MODE_SIZE (limit_mode))
+ best_mode = wider_mode;
+ }
+ return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
+ new_bitnum);
+ }
+ return NULL_RTX;
+}
+
/* Return true if a bitfield of size BITSIZE at bit number BITNUM within
a structure of mode STRUCT_MODE represents a lowpart subreg. The subreg
offset is then BITNUM / BITS_PER_UNIT. */
@@ -432,14 +420,13 @@ lowpart_bit_field_p (unsigned HOST_WIDE_
return bitnum % BITS_PER_WORD == 0;
}
\f
-/* Try to use an insv pattern to store VALUE into a field of OP0.
- OP_MODE is the mode of the insertion and BITSIZE and BITNUM are
- as for store_bit_field. */
+/* Try to use instruction INSV to store VALUE into a field of OP0.
+ BITSIZE and BITNUM are as for store_bit_field. */
static bool
-store_bit_field_using_insv (rtx op0, unsigned HOST_WIDE_INT bitsize,
- unsigned HOST_WIDE_INT bitnum, rtx value,
- enum machine_mode op_mode)
+store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
+ unsigned HOST_WIDE_INT bitsize,
+ unsigned HOST_WIDE_INT bitnum, rtx value)
{
struct expand_operand ops[4];
rtx value1;
@@ -447,13 +434,15 @@ store_bit_field_using_insv (rtx op0, uns
rtx last = get_last_insn ();
bool copy_back = false;
+ enum machine_mode op_mode = insv->field_mode;
unsigned int unit = GET_MODE_BITSIZE (op_mode);
if (bitsize == 0 || bitsize > unit)
return false;
if (MEM_P (xop0))
/* Get a reference to the first byte of the field. */
- xop0 = narrow_bit_field_mem (xop0, byte_mode, bitsize, bitnum, &bitnum);
+ xop0 = narrow_bit_field_mem (xop0, insv->struct_mode, bitsize, bitnum,
+ &bitnum);
else
{
/* Convert from counting within OP0 to counting in OP_MODE. */
@@ -533,7 +522,7 @@ store_bit_field_using_insv (rtx op0, uns
create_integer_operand (&ops[1], bitsize);
create_integer_operand (&ops[2], bitnum);
create_input_operand (&ops[3], value1, op_mode);
- if (maybe_expand_insn (CODE_FOR_insv, 4, ops))
+ if (maybe_expand_insn (insv->icode, 4, ops))
{
if (copy_back)
convert_move (op0, xop0, true);
@@ -807,68 +796,38 @@ store_bit_field_1 (rtx str_rtx, unsigned
within a word. If the destination is a register, it too fits
in a word. */
- enum machine_mode op_mode = mode_for_extraction (EP_insv, 3);
- if (op_mode != MAX_MACHINE_MODE
- && !MEM_P (op0)
- && store_bit_field_using_insv (op0, bitsize, bitnum, value, op_mode))
+ extraction_insn insv;
+ if (!MEM_P (op0)
+ && get_best_reg_extraction_insn (&insv, EP_insv,
+ GET_MODE_BITSIZE (GET_MODE (op0)),
+ fieldmode)
+ && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
return true;
/* If OP0 is a memory, try copying it to a register and seeing if a
cheap register alternative is available. */
- if (op_mode != MAX_MACHINE_MODE && MEM_P (op0))
+ if (MEM_P (op0))
{
- enum machine_mode bestmode;
- unsigned HOST_WIDE_INT maxbits = MAX_FIXED_MODE_SIZE;
-
- /* Do not use insv for volatile bitfields when
- -fstrict-volatile-bitfields is in effect. */
- if (!(MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
- /* Do not use insv if the bit region is restricted and
- an op_mode integer doesn't fit into the restricted region. */
- && !(bitregion_end
- && (bitnum - (bitnum % BITS_PER_UNIT)
- + GET_MODE_BITSIZE (op_mode)
- > bitregion_end + 1))
- && store_bit_field_using_insv (op0, bitsize, bitnum, value, op_mode))
+ /* Do not use unaligned memory insvs for volatile bitfields when
+ -fstrict-volatile-bitfields is in effect. */
+ if (!(MEM_VOLATILE_P (op0)
+ && flag_strict_volatile_bitfields > 0)
+ && get_best_mem_extraction_insn (&insv, EP_insv, bitsize, bitnum,
+ fieldmode)
+ && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
return true;
- if (bitregion_end)
- maxbits = bitregion_end - bitregion_start + 1;
+ rtx last = get_last_insn ();
- /* Get the mode to use for inserting into this field. If OP0 is
- BLKmode, get the smallest mode consistent with the alignment. If
- OP0 is a non-BLKmode object that is no wider than OP_MODE, use its
- mode. Otherwise, use the smallest mode containing the field. */
-
- if (GET_MODE (op0) == BLKmode
- || GET_MODE_BITSIZE (GET_MODE (op0)) > maxbits
- || GET_MODE_SIZE (GET_MODE (op0)) > GET_MODE_SIZE (op_mode))
- bestmode = get_best_mode (bitsize, bitnum,
- bitregion_start, bitregion_end,
- MEM_ALIGN (op0), op_mode,
- MEM_VOLATILE_P (op0));
- else
- bestmode = GET_MODE (op0);
-
- if (bestmode != VOIDmode
- && GET_MODE_SIZE (bestmode) >= GET_MODE_SIZE (fieldmode)
- && !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
- && GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
- {
- rtx last, tempreg, xop0;
- unsigned HOST_WIDE_INT bitpos;
-
- last = get_last_insn ();
-
- /* Adjust address to point to the containing unit of
- that mode. Compute the offset as a multiple of this unit,
- counting in bytes. */
- xop0 = narrow_bit_field_mem (op0, bestmode, bitsize, bitnum,
- &bitpos);
-
- /* Fetch that unit, store the bitfield in it, then store
- the unit. */
- tempreg = copy_to_reg (xop0);
+ /* Try loading part of OP0 into a register, inserting the bitfield
+ into that, and then copying the result back to OP0. */
+ unsigned HOST_WIDE_INT bitpos;
+ rtx xop0 = adjust_bit_field_mem_for_reg (EP_insv, op0, bitsize, bitnum,
+ bitregion_start, bitregion_end,
+ fieldmode, &bitpos);
+ if (xop0)
+ {
+ rtx tempreg = copy_to_reg (xop0);
if (store_bit_field_1 (tempreg, bitsize, bitpos,
bitregion_start, bitregion_end,
fieldmode, orig_value, false))
@@ -913,13 +872,8 @@ store_bit_field (rtx str_rtx, unsigned H
if (MEM_P (str_rtx) && bitregion_start > 0)
{
enum machine_mode bestmode;
- enum machine_mode op_mode;
unsigned HOST_WIDE_INT offset;
- op_mode = mode_for_extraction (EP_insv, 3);
- if (op_mode == MAX_MACHINE_MODE)
- op_mode = VOIDmode;
-
gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
offset = bitregion_start / BITS_PER_UNIT;
@@ -928,8 +882,7 @@ store_bit_field (rtx str_rtx, unsigned H
bitregion_start = 0;
bestmode = get_best_mode (bitsize, bitnum,
bitregion_start, bitregion_end,
- MEM_ALIGN (str_rtx),
- op_mode,
+ MEM_ALIGN (str_rtx), VOIDmode,
MEM_VOLATILE_P (str_rtx));
str_rtx = adjust_address (str_rtx, bestmode, offset);
}
@@ -1251,15 +1204,16 @@ convert_extracted_bit_field (rtx x, enum
are as for extract_bit_field. */
static rtx
-extract_bit_field_using_extv (rtx op0, unsigned HOST_WIDE_INT bitsize,
+extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,
+ unsigned HOST_WIDE_INT bitsize,
unsigned HOST_WIDE_INT bitnum,
int unsignedp, rtx target,
- enum machine_mode mode, enum machine_mode tmode,
- enum machine_mode ext_mode)
+ enum machine_mode mode, enum machine_mode tmode)
{
struct expand_operand ops[4];
rtx spec_target = target;
rtx spec_target_subreg = 0;
+ enum machine_mode ext_mode = extv->field_mode;
unsigned unit = GET_MODE_BITSIZE (ext_mode);
if (bitsize == 0 || unit < bitsize)
@@ -1267,7 +1221,8 @@ extract_bit_field_using_extv (rtx op0, u
if (MEM_P (op0))
/* Get a reference to the first byte of the field. */
- op0 = narrow_bit_field_mem (op0, byte_mode, bitsize, bitnum, &bitnum);
+ op0 = narrow_bit_field_mem (op0, extv->struct_mode, bitsize, bitnum,
+ &bitnum);
else
{
/* Convert from counting within OP0 to counting in EXT_MODE. */
@@ -1315,7 +1270,7 @@ extract_bit_field_using_extv (rtx op0, u
create_fixed_operand (&ops[1], op0);
create_integer_operand (&ops[2], bitsize);
create_integer_operand (&ops[3], bitnum);
- if (maybe_expand_insn (unsignedp ? CODE_FOR_extzv : CODE_FOR_extv, 4, ops))
+ if (maybe_expand_insn (extv->icode, 4, ops))
{
target = ops[0].value;
if (target == spec_target)
@@ -1341,7 +1296,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
{
rtx op0 = str_rtx;
enum machine_mode int_mode;
- enum machine_mode ext_mode;
enum machine_mode mode1;
if (tmode == VOIDmode)
@@ -1612,74 +1566,53 @@ extract_bit_field_1 (rtx str_rtx, unsign
/* From here on we know the desired field is smaller than a word.
If OP0 is a register, it too fits within a word. */
-
- ext_mode = mode_for_extraction (unsignedp ? EP_extzv : EP_extv, 0);
- if (ext_mode != MAX_MACHINE_MODE && !MEM_P (op0))
+ enum extraction_pattern pattern = unsignedp ? EP_extzv : EP_extv;
+ extraction_insn extv;
+ if (!MEM_P (op0)
+ && get_best_reg_extraction_insn (&extv, pattern, bitnum + bitsize,
+ tmode))
{
- rtx result = extract_bit_field_using_extv (op0, bitsize, bitnum,
+ rtx result = extract_bit_field_using_extv (&extv, op0, bitsize, bitnum,
unsignedp, target, mode,
- tmode, ext_mode);
+ tmode);
if (result)
return result;
}
/* If OP0 is a memory, try copying it to a register and seeing if a
cheap register alternative is available. */
- if (ext_mode != MAX_MACHINE_MODE && MEM_P (op0))
+ if (MEM_P (op0))
{
- enum machine_mode bestmode;
-
/* Do not use extv/extzv for volatile bitfields when
-fstrict-volatile-bitfields is in effect. */
- if (!(MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0))
+ if (!(MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
+ && get_best_mem_extraction_insn (&extv, pattern, bitsize, bitnum,
+ tmode))
{
- rtx result = extract_bit_field_using_extv (op0, bitsize, bitnum,
- unsignedp, target, mode,
- tmode, ext_mode);
+ rtx result = extract_bit_field_using_extv (&extv, op0, bitsize,
+ bitnum, unsignedp,
+ target, mode,
+ tmode);
if (result)
return result;
}
- /* Get the mode to use for inserting into this field. If
- OP0 is BLKmode, get the smallest mode consistent with the
- alignment. If OP0 is a non-BLKmode object that is no
- wider than EXT_MODE, use its mode. Otherwise, use the
- smallest mode containing the field. */
-
- if (GET_MODE (op0) == BLKmode
- || GET_MODE_SIZE (GET_MODE (op0)) > GET_MODE_SIZE (ext_mode))
- bestmode = get_best_mode (bitsize, bitnum, 0, 0, MEM_ALIGN (op0),
- ext_mode, MEM_VOLATILE_P (op0));
- else
- bestmode = GET_MODE (op0);
-
- if (bestmode != VOIDmode
- && !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
- && GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
- {
- unsigned HOST_WIDE_INT bitpos;
- rtx xop0 = narrow_bit_field_mem (op0, bestmode, bitsize, bitnum,
- &bitpos);
-
- /* Make sure the register is big enough for the whole field.
- (It might not be if bestmode == GET_MODE (op0) and the input
- code was invalid.) */
- if (bitpos + bitsize <= GET_MODE_BITSIZE (bestmode))
- {
- rtx last, result;
-
- last = get_last_insn ();
+ rtx last = get_last_insn ();
- /* Fetch it to a register in that size. */
- xop0 = force_reg (bestmode, xop0);
- result = extract_bit_field_1 (xop0, bitsize, bitpos,
+ /* Try loading part of OP0 into a register and extracting the
+ bitfield from that. */
+ unsigned HOST_WIDE_INT bitpos;
+ rtx xop0 = adjust_bit_field_mem_for_reg (pattern, op0, bitsize, bitnum,
+ 0, 0, tmode, &bitpos);
+ if (xop0)
+ {
+ xop0 = copy_to_reg (xop0);
+ rtx result = extract_bit_field_1 (xop0, bitsize, bitpos,
unsignedp, packedp, target,
mode, tmode, false);
- if (result)
- return result;
-
- delete_insns_since (last);
- }
+ if (result)
+ return result;
+ delete_insns_since (last);
}
}
Index: gcc/recog.c
===================================================================
--- gcc/recog.c 2012-11-03 09:21:20.664369114 +0000
+++ gcc/recog.c 2012-11-03 11:32:18.694349886 +0000
@@ -40,6 +40,7 @@ Software Foundation; either version 3, o
#include "target.h"
#include "tree-pass.h"
#include "df.h"
+#include "insn-codes.h"
#ifndef STACK_PUSH_CODE
#ifdef STACK_GROWS_DOWNWARD
@@ -550,6 +551,16 @@ cancel_changes (int num)
num_changes = num;
}
+/* Reduce conditional compilation elsewhere. */
+#ifndef HAVE_extv
+#define HAVE_extv 0
+#define CODE_FOR_extv CODE_FOR_nothing
+#endif
+#ifndef HAVE_extzv
+#define HAVE_extzv 0
+#define CODE_FOR_extzv CODE_FOR_nothing
+#endif
+
/* A subroutine of validate_replace_rtx_1 that tries to simplify the resulting
rtx. */
@@ -637,19 +648,17 @@ simplify_while_replacing (rtx *loc, rtx
enum machine_mode is_mode = GET_MODE (XEXP (x, 0));
int pos = INTVAL (XEXP (x, 2));
- if (GET_CODE (x) == ZERO_EXTRACT)
+ if (GET_CODE (x) == ZERO_EXTRACT && HAVE_extzv)
{
- enum machine_mode new_mode
- = mode_for_extraction (EP_extzv, 1);
- if (new_mode != MAX_MACHINE_MODE)
- wanted_mode = new_mode;
+ wanted_mode = insn_data[CODE_FOR_extzv].operand[1].mode;
+ if (wanted_mode == VOIDmode)
+ wanted_mode = word_mode;
}
- else if (GET_CODE (x) == SIGN_EXTRACT)
+ else if (GET_CODE (x) == SIGN_EXTRACT && HAVE_extv)
{
- enum machine_mode new_mode
- = mode_for_extraction (EP_extv, 1);
- if (new_mode != MAX_MACHINE_MODE)
- wanted_mode = new_mode;
+ wanted_mode = insn_data[CODE_FOR_extv].operand[1].mode;
+ if (wanted_mode == VOIDmode)
+ wanted_mode = word_mode;
}
/* If we have a narrower mode, we can do something. */
^ permalink raw reply [flat|nested] 42+ messages in thread
* [8/8] Add new optabs and use them for MIPS
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (6 preceding siblings ...)
2012-11-03 11:39 ` [7/8] Replace mode_for_extraction with new interface Richard Sandiford
@ 2012-11-03 11:41 ` Richard Sandiford
2012-11-27 17:11 ` [0/8] Add optabs alternatives for insv, extv and extzv Ramana Radhakrishnan
8 siblings, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:41 UTC (permalink / raw)
To: gcc-patches
This patch adds the new optabs and makes MIPS use them.
There are probably some clean-up opportunities in mips_use_ins_ext_p
after this change, but I'll look at that separately.
Tested as described in the covering note. OK to install?
Richard
gcc/
* doc/md.texi (extv@var{m}, extvmisalign@var{m}, extzv@var{m})
(extzvmisalign@var{m}, insv@var{m}, insvmisalign@var{m}): Document.
(insv, extv, extzv): Deprecate.
* optabs.def (insv_optab, extv_optab, extzv_optab)
(insvmisalign_optab, extvmisalign_optab, extzvmisalign_optab):
New optabs.
* optabs.c (get_optab_extraction_insn): New function.
(get_extraction_insn): Use it.
* config/mips/mips.md (extv): Split into...
(extvmisalign<mode>, extv<mode>): ...these new patterns. Rename
existing extv<mode> pattern to...
(*extv<mode>): ...this.
(extzv): Split into...
(extzvmisalign<mode>, extzv<mode>): ...these new patterns. Rename
existing extzv<mode> pattern to...
(*extzv<mode>): ...this.
(insv): Split into...
(insvmisalign<mode>, insv<mode>): ...these new patterns. Rename
existing insv<mode> pattern to...
(*insv<mode>): ...this. Use const_int_operand rather than
immediate_operand.
* config/mips/mips.c (mips_block_move_straight): Use set_mem_size
to set the size of BLKmode accesses.
(mips_get_unaligned_mem): Require OP0 to be a BLKmode memory,
turning it from an "rtx *" to an rtx.
(mips_expand_ext_as_unaligned_load): Simplify for new optab
interface. Update call to mips_get_unaligned_mem.
(mips_expand_ins_as_unaligned_store): Update call to
mips_get_unaligned_mem.
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi 2012-11-02 22:43:15.315373159 +0000
+++ gcc/doc/md.texi 2012-11-02 23:33:53.633365725 +0000
@@ -5288,6 +5288,62 @@ Convert unsigned integer operand 1 of mo
When overflows or underflows happen, the instruction saturates the
results to the maximum or the minimum.
+@cindex @code{extv@var{m}} instruction pattern
+@item @samp{extv@var{m}}
+Extract a bit-field from register operand 1, sign-extend it, and store
+it in operand 0. Operand 2 specifies the width of the field in bits
+and operand 3 the starting bit, which counts from the most significant
+bit if @samp{BITS_BIG_ENDIAN} is true and from the least significant bit
+otherwise.
+
+Operands 0 and 1 both have mode @var{m}. Operands 2 and 3 have a
+target-specific mode.
+
+@cindex @code{extvmisalign@var{m}} instruction pattern
+@item @samp{extvmisalign@var{m}}
+Extract a bit-field from memory operand 1, sign extend it, and store
+it in operand 0. Operand 2 specifies the width in bits and operand 3
+the starting bit. The starting bit is always somewhere in the first byte of
+operand 1; it counts from the most significant bit if @samp{BITS_BIG_ENDIAN}
+is true and from the least significant bit otherwise.
+
+Operand 0 has mode @var{m} while operand 1 has @code{BLK} mode.
+Operands 2 and 3 have a target-specific mode.
+
+The instruction must not read beyond the last byte of the bit-field.
+
+@cindex @code{extzv@var{m}} instruction pattern
+@item @samp{extzv@var{m}}
+Like @samp{extv@var{m}} except that the bit-field value is zero-extended.
+
+@cindex @code{extzvmisalign@var{m}} instruction pattern
+@item @samp{extzvmisalign@var{m}}
+Like @samp{extvmisalign@var{m}} except that the bit-field value is
+zero-extended.
+
+@cindex @code{insv@var{m}} instruction pattern
+@item @samp{insv@var{m}}
+Insert operand 3 into a bit-field of register operand 0. Operand 1
+specifies the width of the field in bits and operand 2 the starting bit,
+which counts from the most significant bit if @samp{BITS_BIG_ENDIAN}
+is true and from the least significant bit otherwise.
+
+Operands 0 and 3 both have mode @var{m}. Operands 1 and 2 have a
+target-specific mode.
+
+@cindex @code{insvmisalign@var{m}} instruction pattern
+@item @samp{insvmisalign@var{m}}
+Insert operand 3 into a bit-field of memory operand 0. Operand 1
+specifies the width of the field in bits and operand 2 the starting bit.
+The starting bit is always somewhere in the first byte of operand 0;
+it counts from the most significant bit if @samp{BITS_BIG_ENDIAN}
+is true and from the least significant bit otherwise.
+
+Operand 3 has mode @var{m} while operand 0 has @code{BLK} mode.
+Operands 1 and 2 have a target-specific mode.
+
+The instruction must not read or write beyond the last byte of the bit-field.
+
@cindex @code{extv} instruction pattern
@item @samp{extv}
Extract a bit-field from operand 1 (a register or memory operand), where
@@ -5303,10 +5359,16 @@ for operands 2 and 3 and the constant is
The bit-field value is sign-extended to a full word integer
before it is stored in operand 0.
+This pattern is deprecated; please use @samp{extv@var{m}} and
+@code{extvmisalign@var{m}} instead.
+
@cindex @code{extzv} instruction pattern
@item @samp{extzv}
Like @samp{extv} except that the bit-field value is zero-extended.
+This pattern is deprecated; please use @samp{extzv@var{m}} and
+@code{extzvmisalign@var{m}} instead.
+
@cindex @code{insv} instruction pattern
@item @samp{insv}
Store operand 3 (which must be valid for @code{word_mode}) into a
@@ -5318,6 +5380,9 @@ Operands 1 and 2 must be valid for @code
The RTL generation pass generates this instruction only with constants
for operands 1 and 2 and the constant is never zero for operand 1.
+This pattern is deprecated; please use @samp{insv@var{m}} and
+@code{insvmisalign@var{m}} instead.
+
@cindex @code{mov@var{mode}cc} instruction pattern
@item @samp{mov@var{mode}cc}
Conditionally move operand 2 or operand 3 into operand 0 according to the
Index: gcc/optabs.def
===================================================================
--- gcc/optabs.def 2012-11-02 22:43:15.314373159 +0000
+++ gcc/optabs.def 2012-11-02 23:33:53.622365725 +0000
@@ -171,6 +171,12 @@ OPTAB_DC(mov_optab, "mov$a", SET)
OPTAB_DC(movstrict_optab, "movstrict$a", STRICT_LOW_PART)
OPTAB_D (movmisalign_optab, "movmisalign$a")
OPTAB_D (storent_optab, "storent$a")
+OPTAB_D (insv_optab, "insv$a")
+OPTAB_D (extv_optab, "extv$a")
+OPTAB_D (extzv_optab, "extzv$a")
+OPTAB_D (insvmisalign_optab, "insvmisalign$a")
+OPTAB_D (extvmisalign_optab, "extvmisalign$a")
+OPTAB_D (extzvmisalign_optab, "extzvmisalign$a")
OPTAB_D (push_optab, "push$a1")
OPTAB_D (reload_in_optab, "reload_in$a")
OPTAB_D (reload_out_optab, "reload_out$a")
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c 2012-11-02 23:25:16.059366992 +0000
+++ gcc/optabs.c 2012-11-02 23:36:03.255365408 +0000
@@ -8296,6 +8296,35 @@ get_traditional_extraction_insn (extract
return true;
}
+/* Return true if an optab exists to perform an insertion or extraction
+ of type TYPE in mode MODE. Describe the instruction in *INSN if so.
+
+ REG_OPTAB is the optab to use for register structures and
+ MISALIGN_OPTAB is the optab to use for misaligned memory structures.
+ POS_OP is the operand number of the bit position. */
+
+static bool
+get_optab_extraction_insn (struct extraction_insn *insn,
+ enum extraction_type type,
+ enum machine_mode mode, direct_optab reg_optab,
+ direct_optab misalign_optab, int pos_op)
+{
+ direct_optab optab = (type == ET_unaligned_mem ? misalign_optab : reg_optab);
+ enum insn_code icode = direct_optab_handler (optab, mode);
+ if (icode == CODE_FOR_nothing)
+ return false;
+
+ const struct insn_data_d *data = &insn_data[icode];
+
+ insn->icode = icode;
+ insn->field_mode = mode;
+ insn->struct_mode = (type == ET_unaligned_mem ? BLKmode : mode);
+ insn->pos_mode = data->operand[pos_op].mode;
+ if (insn->pos_mode == VOIDmode)
+ insn->pos_mode = word_mode;
+ return true;
+}
+
/* Return true if an instruction exists to perform an insertion or
extraction (PATTERN says which) of type TYPE on a structure of mode MODE.
Describe the instruction in *INSN if so. */
@@ -8313,21 +8342,24 @@ get_extraction_insn (extraction_insn *in
&& get_traditional_extraction_insn (insn, type, mode,
CODE_FOR_insv, 0, 3))
return true;
- return false;
+ return get_optab_extraction_insn (insn, type, mode, insv_optab,
+ insvmisalign_optab, 2);
case EP_extv:
if (HAVE_extv
&& get_traditional_extraction_insn (insn, type, mode,
CODE_FOR_extv, 1, 0))
return true;
- return false;
+ return get_optab_extraction_insn (insn, type, mode, extv_optab,
+ extvmisalign_optab, 3);
case EP_extzv:
if (HAVE_extzv
&& get_traditional_extraction_insn (insn, type, mode,
CODE_FOR_extzv, 1, 0))
return true;
- return false;
+ return get_optab_extraction_insn (insn, type, mode, extzv_optab,
+ extzvmisalign_optab, 3);
default:
gcc_unreachable ();
Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md 2012-11-02 22:43:15.316373159 +0000
+++ gcc/config/mips/mips.md 2012-11-02 23:33:53.628365725 +0000
@@ -3777,11 +3777,11 @@ (define_expand "fixuns_truncsfdi2"
;; Bit field extract patterns which use lwl/lwr or ldl/ldr.
-(define_expand "extv"
- [(set (match_operand 0 "register_operand")
- (sign_extract (match_operand 1 "nonimmediate_operand")
- (match_operand 2 "const_int_operand")
- (match_operand 3 "const_int_operand")))]
+(define_expand "extvmisalign<mode>"
+ [(set (match_operand:GPR 0 "register_operand")
+ (sign_extract:GPR (match_operand:BLK 1 "memory_operand")
+ (match_operand 2 "const_int_operand")
+ (match_operand 3 "const_int_operand")))]
"!TARGET_MIPS16"
{
if (mips_expand_ext_as_unaligned_load (operands[0], operands[1],
@@ -3789,22 +3789,22 @@ (define_expand "extv"
INTVAL (operands[3]),
/*unsigned=*/ false))
DONE;
- else if (register_operand (operands[1], GET_MODE (operands[0]))
- && ISA_HAS_EXTS && UINTVAL (operands[2]) <= 32)
- {
- if (GET_MODE (operands[0]) == DImode)
- emit_insn (gen_extvdi (operands[0], operands[1], operands[2],
- operands[3]));
- else
- emit_insn (gen_extvsi (operands[0], operands[1], operands[2],
- operands[3]));
- DONE;
- }
else
FAIL;
})
-(define_insn "extv<mode>"
+(define_expand "extv<mode>"
+ [(set (match_operand:GPR 0 "register_operand")
+ (sign_extract:GPR (match_operand:GPR 1 "register_operand")
+ (match_operand 2 "const_int_operand")
+ (match_operand 3 "const_int_operand")))]
+ "ISA_HAS_EXTS"
+{
+ if (UINTVAL (operands[2]) > 32)
+ FAIL;
+})
+
+(define_insn "*extv<mode>"
[(set (match_operand:GPR 0 "register_operand" "=d")
(sign_extract:GPR (match_operand:GPR 1 "register_operand" "d")
(match_operand 2 "const_int_operand" "")
@@ -3814,35 +3814,35 @@ (define_insn "extv<mode>"
[(set_attr "type" "arith")
(set_attr "mode" "<MODE>")])
-
-(define_expand "extzv"
- [(set (match_operand 0 "register_operand")
- (zero_extract (match_operand 1 "nonimmediate_operand")
- (match_operand 2 "const_int_operand")
- (match_operand 3 "const_int_operand")))]
+(define_expand "extzvmisalign<mode>"
+ [(set (match_operand:GPR 0 "register_operand")
+ (zero_extract:GPR (match_operand:BLK 1 "memory_operand")
+ (match_operand 2 "const_int_operand")
+ (match_operand 3 "const_int_operand")))]
"!TARGET_MIPS16"
{
if (mips_expand_ext_as_unaligned_load (operands[0], operands[1],
INTVAL (operands[2]),
INTVAL (operands[3]),
- /*unsigned=*/true))
+ /*unsigned=*/ true))
DONE;
- else if (mips_use_ins_ext_p (operands[1], INTVAL (operands[2]),
- INTVAL (operands[3])))
- {
- if (GET_MODE (operands[0]) == DImode)
- emit_insn (gen_extzvdi (operands[0], operands[1], operands[2],
- operands[3]));
- else
- emit_insn (gen_extzvsi (operands[0], operands[1], operands[2],
- operands[3]));
- DONE;
- }
else
FAIL;
})
-(define_insn "extzv<mode>"
+(define_expand "extzv<mode>"
+ [(set (match_operand:GPR 0 "register_operand")
+ (zero_extract:GPR (match_operand:GPR 1 "register_operand")
+ (match_operand 2 "const_int_operand")
+ (match_operand 3 "const_int_operand")))]
+ ""
+{
+ if (!mips_use_ins_ext_p (operands[1], INTVAL (operands[2]),
+ INTVAL (operands[3])))
+ FAIL;
+})
+
+(define_insn "*extzv<mode>"
[(set (match_operand:GPR 0 "register_operand" "=d")
(zero_extract:GPR (match_operand:GPR 1 "register_operand" "d")
(match_operand 2 "const_int_operand" "")
@@ -3865,36 +3865,37 @@ (define_insn "*extzv_truncsi_exts"
(set_attr "mode" "SI")])
-(define_expand "insv"
- [(set (zero_extract (match_operand 0 "nonimmediate_operand")
- (match_operand 1 "immediate_operand")
- (match_operand 2 "immediate_operand"))
- (match_operand 3 "reg_or_0_operand"))]
+(define_expand "insvmisalign<mode>"
+ [(set (zero_extract:GPR (match_operand:BLK 0 "memory_operand")
+ (match_operand 1 "const_int_operand")
+ (match_operand 2 "const_int_operand"))
+ (match_operand:GPR 3 "reg_or_0_operand"))]
"!TARGET_MIPS16"
{
if (mips_expand_ins_as_unaligned_store (operands[0], operands[3],
INTVAL (operands[1]),
INTVAL (operands[2])))
DONE;
- else if (mips_use_ins_ext_p (operands[0], INTVAL (operands[1]),
- INTVAL (operands[2])))
- {
- if (GET_MODE (operands[0]) == DImode)
- emit_insn (gen_insvdi (operands[0], operands[1], operands[2],
- operands[3]));
- else
- emit_insn (gen_insvsi (operands[0], operands[1], operands[2],
- operands[3]));
- DONE;
- }
- else
- FAIL;
+ else
+ FAIL;
+})
+
+(define_expand "insv<mode>"
+ [(set (zero_extract:GPR (match_operand:GPR 0 "register_operand")
+ (match_operand 1 "const_int_operand")
+ (match_operand 2 "const_int_operand"))
+ (match_operand:GPR 3 "reg_or_0_operand"))]
+ ""
+{
+ if (!mips_use_ins_ext_p (operands[0], INTVAL (operands[1]),
+ INTVAL (operands[2])))
+ FAIL;
})
-(define_insn "insv<mode>"
+(define_insn "*insv<mode>"
[(set (zero_extract:GPR (match_operand:GPR 0 "register_operand" "+d")
- (match_operand:SI 1 "immediate_operand" "I")
- (match_operand:SI 2 "immediate_operand" "I"))
+ (match_operand:SI 1 "const_int_operand" "")
+ (match_operand:SI 2 "const_int_operand" ""))
(match_operand:GPR 3 "reg_or_0_operand" "dJ"))]
"mips_use_ins_ext_p (operands[0], INTVAL (operands[1]),
INTVAL (operands[2]))"
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c 2012-11-02 22:43:15.316373159 +0000
+++ gcc/config/mips/mips.c 2012-11-02 23:33:53.647365725 +0000
@@ -7101,6 +7101,7 @@ mips_block_move_straight (rtx dest, rtx
else
{
rtx part = adjust_address (src, BLKmode, offset);
+ set_mem_size (part, delta);
if (!mips_expand_ext_as_unaligned_load (regs[i], part, bits, 0, 0))
gcc_unreachable ();
}
@@ -7113,6 +7114,7 @@ mips_block_move_straight (rtx dest, rtx
else
{
rtx part = adjust_address (dest, BLKmode, offset);
+ set_mem_size (part, delta);
if (!mips_expand_ins_as_unaligned_store (part, regs[i], bits, 0))
gcc_unreachable ();
}
@@ -7364,10 +7366,8 @@ mips_expand_atomic_qihi (union mips_gen_
}
/* Return true if it is possible to use left/right accesses for a
- bitfield of WIDTH bits starting BITPOS bits into *OP. When
- returning true, update *OP, *LEFT and *RIGHT as follows:
-
- *OP is a BLKmode reference to the whole field.
+ bitfield of WIDTH bits starting BITPOS bits into BLKmode memory OP.
+ When returning true, update *LEFT and *RIGHT as follows:
*LEFT is a QImode reference to the first byte if big endian or
the last byte if little endian. This address can be used in the
@@ -7377,16 +7377,11 @@ mips_expand_atomic_qihi (union mips_gen_
can be used in the patterning right-side instruction. */
static bool
-mips_get_unaligned_mem (rtx *op, HOST_WIDE_INT width, HOST_WIDE_INT bitpos,
+mips_get_unaligned_mem (rtx op, HOST_WIDE_INT width, HOST_WIDE_INT bitpos,
rtx *left, rtx *right)
{
rtx first, last;
- /* Check that the operand really is a MEM. Not all the extv and
- extzv predicates are checked. */
- if (!MEM_P (*op))
- return false;
-
/* Check that the size is valid. */
if (width != 32 && (!TARGET_64BIT || width != 64))
return false;
@@ -7399,20 +7394,12 @@ mips_get_unaligned_mem (rtx *op, HOST_WI
/* Reject aligned bitfields: we want to use a normal load or store
instead of a left/right pair. */
- if (MEM_ALIGN (*op) >= width)
+ if (MEM_ALIGN (op) >= width)
return false;
- /* Create a copy of *OP that refers to the whole field. This also has
- the effect of legitimizing *OP's address for BLKmode, possibly
- simplifying it. */
- *op = copy_rtx (adjust_address (*op, BLKmode, 0));
- set_mem_size (*op, width / BITS_PER_UNIT);
-
- /* Get references to both ends of the field. We deliberately don't
- use the original QImode *OP for FIRST since the new BLKmode one
- might have a simpler address. */
- first = adjust_address (*op, QImode, 0);
- last = adjust_address (*op, QImode, width / BITS_PER_UNIT - 1);
+ /* Get references to both ends of the field. */
+ first = adjust_address (op, QImode, 0);
+ last = adjust_address (op, QImode, width / BITS_PER_UNIT - 1);
/* Allocate to LEFT and RIGHT according to endianness. LEFT should
correspond to the MSB and RIGHT to the LSB. */
@@ -7440,14 +7427,6 @@ mips_expand_ext_as_unaligned_load (rtx d
rtx dest1 = NULL_RTX;
/* If TARGET_64BIT, the destination of a 32-bit "extz" or "extzv" will
- be a paradoxical word_mode subreg. This is the only case in which
- we allow the destination to be larger than the source. */
- if (GET_CODE (dest) == SUBREG
- && GET_MODE (dest) == DImode
- && GET_MODE (SUBREG_REG (dest)) == SImode)
- dest = SUBREG_REG (dest);
-
- /* If TARGET_64BIT, the destination of a 32-bit "extz" or "extzv" will
be a DImode, create a new temp and emit a zero extend at the end. */
if (GET_MODE (dest) == DImode
&& REG_P (dest)
@@ -7457,12 +7436,7 @@ mips_expand_ext_as_unaligned_load (rtx d
dest = gen_reg_rtx (SImode);
}
- /* After the above adjustment, the destination must be the same
- width as the source. */
- if (GET_MODE_BITSIZE (GET_MODE (dest)) != width)
- return false;
-
- if (!mips_get_unaligned_mem (&src, width, bitpos, &left, &right))
+ if (!mips_get_unaligned_mem (src, width, bitpos, &left, &right))
return false;
temp = gen_reg_rtx (GET_MODE (dest));
@@ -7504,7 +7478,7 @@ mips_expand_ins_as_unaligned_store (rtx
rtx left, right;
enum machine_mode mode;
- if (!mips_get_unaligned_mem (&dest, width, bitpos, &left, &right))
+ if (!mips_get_unaligned_mem (dest, width, bitpos, &left, &right))
return false;
mode = mode_for_size (width, MODE_INT, 0);
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [1/8] Handle TRUNCATE in make_extraction
2012-11-03 11:13 ` [1/8] Handle TRUNCATE in make_extraction Richard Sandiford
@ 2012-11-10 15:52 ` Eric Botcazou
0 siblings, 0 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-10 15:52 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> Tested as described in the covering note. OK to install?
>
> Richard
>
> gcc/
> * combine.c (make_extraction): Handle TRUNCATEd INNERs.
OK, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [2/8] Add adjust_bitfield_address_size
2012-11-03 11:14 ` [2/8] Add adjust_bitfield_address_size Richard Sandiford
@ 2012-11-10 15:53 ` Eric Botcazou
0 siblings, 0 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-10 15:53 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> Tested as described in the covering note. OK to install?
>
> Richard
>
>
> gcc/
> * expr.h (adjust_address_1): Add a size parameter.
> (adjust_address, adjust_address_nv, adjust_bitfield_address)
> (adjust_bitfield_address_nv): Adjust accordingly.
> (adjust_bitfield_address_size): Define.
> * emit-rtl.c (adjust_address_1): Add a size parameter.
> Use it to set the size if MODE has no size. Check whether
> the size matches before returning the original memref.
> Require the size to be known for adjust_object.
> (adjust_automodify_address_1, widen_memory_access): Update calls
> to adjust_address_1.
No objections by me.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [3/8] Add narrow_bit_field_mem
2012-11-03 11:16 ` [3/8] Add narrow_bit_field_mem Richard Sandiford
@ 2012-11-10 16:02 ` Eric Botcazou
0 siblings, 0 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-10 16:02 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> Tested as described in the covering note. OK to install?
>
> Richard
>
>
> gcc/
> * expmed.c (narrow_bit_field_mem): New function.
> (store_bit_field_using_insv, store_bit_field_1, store_fixed_bit_field)
> (extract_bit_field_1): Use it.
This looks better now, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-03 11:21 ` [4/8] Add bit_field_mode_iterator Richard Sandiford
@ 2012-11-13 12:44 ` Eric Botcazou
2012-11-13 21:46 ` Richard Henderson
2012-11-18 17:36 ` Richard Sandiford
0 siblings, 2 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-13 12:44 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> get_best_mode has various checks to decide what counts as an acceptable
> bitfield mode. It actually has two copies of them, with slightly different
> alignment checks:
>
> MIN (unit, BIGGEST_ALIGNMENT) > align
>
> vs.
>
> unit <= MIN (align, BIGGEST_ALIGNMENT)
>
> The second looks more correct, since we can't necessarily guarantee
> larger alignments than BIGGEST_ALIGNMENT in all cases.
Under the assumption that integer modes really require maximal alignment, i.e.
whatever BIGGEST_ALIGNMENT is, I agree.
> This patch adds a new iterator class that can be used to walk through
> the modes, and rewrites get_best_mode to use it. I kept the existing
> checks with two changes:
>
> - bitregion_start is now tested independently of bitregion_end
The comments needs to be updated then.
> - MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined
This makes sense I think.
> It shouldn't make any difference in practice, but both changes felt
> more in keeping with the documentation of bitregion_start and
> MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
> test to be separate from bitregion_start.
>
> The behaviour of the Sequent i386 compiler probably isn't the
> issue it once was, but that's also dealt with in the next patch.
>
> Tested as described in the covering note. OK to install?
>
> Richard
>
>
> gcc/
> * machmode.h (bit_field_mode_iterator): New class.
> (get_best_mode): Change final parameter to bool.
> * stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
> (bit_field_mode_iterator::next_mode): New functions, split out from...
> (get_best_mode): ...here. Change final parameter to bool.
> Use bit_field_mode_iterator.
This looks good to me, modulo:
> + volatilep_ (volatilep), count_ (0)
> +{
> + if (bitregion_end_)
> + bitregion_end_ += 1;
> +}
IMO this is confusing. I think bitregion_end/bitregion_end_ should have a
consistent meaning.
> +/* Calls to this function return successively larger modes that can be used
> + to represent the bitfield. Return true if another bitfield mode is +
> available, storing it in *OUT_MODE if so. */
> +
> +bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)
'bool' on its own line I think.
I find the interface a bit awkward though. Can't we model it on the existing
iterators in basic-block.h or tree-flow.h? get_best_mode would be written:
FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
bitregion_start, bitregion_end,
align, volatilep)
{
if (largest_mode != VOIDmode
&& GET_MODE_SIZE (mode) > GET_MODE_SIZE (largest_mode)
break;
if (iter.prefer_smaller_modes ())
return mode;
widest_mode = mode;
}
return widest_mode;
and the implementation entirely hidden.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-03 11:27 ` [5/8] Tweak bitfield alignment handling Richard Sandiford
@ 2012-11-13 13:52 ` Eric Botcazou
2012-11-18 17:36 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-13 13:52 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> This patch replaces:
>
> /* Stop if the mode is wider than the alignment of the containing
> object.
>
> It is tempting to omit the following line unless STRICT_ALIGNMENT
> is true. But that is incorrect, since if the bitfield uses part
> of 3 bytes and we use a 4-byte mode, we could get a spurious segv
> if the extra 4th byte is past the end of memory.
> (Though at least one Unix compiler ignores this problem:
> that on the Sequent 386 machine. */
> if (unit > align_)
> break;
>
> with two checks: one for whether the final byte of the mode is known
> to be mapped, and one for whether the bitfield is sufficiently aligned.
> Later patches in the series rely on this in order not to pessimise
> memory handling.
>
> However, as described in the patch, I found that extending this
> behaviour to get_best_mode affected code generation for x86_64-linux-gnu
> and powerpc64-linux-gnu, and led to a failure in bb-slp-5.c on both.
> I therefore chickened out and added the original check back to
> get_best_mode.
>
> I'm certainly interested in any feedback on the comment, but at the
> same time I'd like this series to be a no-op on targets that keep
> the traditional .md patterns. Any change to get_best_mode is probably
> best done as a separate patch.
I agree with your conservative approach here.
> gcc/
> * stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
> Set up a default value of bitregion_end_.
> (bit_field_mode_iterator::next_mode): Always apply bitregion_end_
> check. Include SLOW_UNALIGNED_ACCESS in the alignment check.
> (get_best_mode): Ignore modes that are wider than the alignment.
Fine with me, modulo:
> @@ -2754,6 +2753,62 @@ get_best_mode (int bitsize, int bitpos,
> enum machine_mode widest_mode = VOIDmode;
> enum machine_mode mode;
> while (iter.next_mode (&mode)
> + /* ??? For compatiblity, reject modes that are wider than the
> + alignment. This has both advantages and disadvantages.
"For compatibility" is a bit vague (compatibility with what?). I'd write "For
historical reasons" or something along these lines.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-03 11:28 ` [6/8] Add strict volatile handling to bit_field_mode_iterator Richard Sandiford
@ 2012-11-13 13:57 ` Eric Botcazou
2012-11-15 12:25 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-13 13:57 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
> into account, in cases where the size of the underlying object is known.
> This is used in the next patch.
Do we really need to add that to the iterator? The -fstrict-volatile-
bitfields implementation is still controversial so I'm not sure that we want
let it spread. Can't the client code just skip the problematic modes?
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-13 12:44 ` Eric Botcazou
@ 2012-11-13 21:46 ` Richard Henderson
2012-11-13 22:05 ` Eric Botcazou
2012-11-18 17:36 ` Richard Sandiford
1 sibling, 1 reply; 42+ messages in thread
From: Richard Henderson @ 2012-11-13 21:46 UTC (permalink / raw)
To: Eric Botcazou; +Cc: Richard Sandiford, gcc-patches
On 11/13/2012 04:41 AM, Eric Botcazou wrote:
> I find the interface a bit awkward though. Can't we model it on the existing
> iterators in basic-block.h or tree-flow.h? get_best_mode would be written:
>
> FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
> bitregion_start, bitregion_end,
> align, volatilep)
Now that we're in C++, I think we should be using iterators that are much
more in the style of libstdc++. I agree that the .next interface used here
is a bit confusing.
I'd expect to see something like
for (bit_field_mode_iterator i(x,y,z); i ; ++i)
{
enum machine_mode mode = *i;
...
}
For 4.9, I expect that someone can slowly purge our FOR_EACH_* macros...
r~
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-13 21:46 ` Richard Henderson
@ 2012-11-13 22:05 ` Eric Botcazou
2012-11-15 12:11 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-13 22:05 UTC (permalink / raw)
To: Richard Henderson; +Cc: gcc-patches, Richard Sandiford
> Now that we're in C++, I think we should be using iterators that are much
> more in the style of libstdc++. I agree that the .next interface used here
> is a bit confusing.
>
> I'd expect to see something like
>
> for (bit_field_mode_iterator i(x,y,z); i ; ++i)
> {
> enum machine_mode mode = *i;
> ...
> }
I pondered on that for half an hour. :-) But the amount of stuff you need to
write to make it work in this particular case will make the implementation
convoluted and bloated for no obvious gains IMO.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-13 22:05 ` Eric Botcazou
@ 2012-11-15 12:11 ` Richard Sandiford
2012-11-15 20:39 ` Richard Henderson
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-15 12:11 UTC (permalink / raw)
To: Eric Botcazou; +Cc: Richard Henderson, gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> Now that we're in C++, I think we should be using iterators that are much
>> more in the style of libstdc++. I agree that the .next interface used here
>> is a bit confusing.
>>
>> I'd expect to see something like
>>
>> for (bit_field_mode_iterator i(x,y,z); i ; ++i)
>> {
>> enum machine_mode mode = *i;
>> ...
>> }
>
> I pondered on that for half an hour. :-) But the amount of stuff you need to
> write to make it work in this particular case will make the implementation
> convoluted and bloated for no obvious gains IMO.
Yeah, I'd also thought about doing it as a "proper" iterator, and came
to the same conclusion. Although I was thinking of:
bit_field_def def (blah...);
for (bit_field_def::mode_iterator i = def.modes_begin();
i != def.modes_end();
++i)
{
enum machine_mode mode = *i;
...
}
Then presumably we'd need all the other iterator gloss too
(value_type, etc.).
I hadn't thought about an operator bool terminator. I agree that's
probably simpler, but do any libstdc++ classes have the same thing?
It doesn't feel any more standard than the "while (get_more)" idiom to me,
but that's probably just my ignorance of C++.
My problem with:
FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
bitregion_start, bitregion_end,
align, volatilep)
is that there are too many parameters to make it obvious what we're
actually iterating over. I.e. these FOR_EACH_* macros tend to have a
combination of three things: inputs, "internal" iterator state that
nevertheless needs to be defined separately in order to avoid surprising
name clashes, and the iterator variable itself. When there's a lot of
parameters, it's not always obvious which is which.
Also, it makes it harder to implement loops like:
setup
if (get first mode)
if (want to search for more)
while (more modes...)
which is the structure used in patch 7.
Admittedly both "problems" apply to some of the existing FOR_EACH_*
macros too, so there's definitely a consistency argument in favour of
doing this anyway.
"next" was supposed to be "find and return another mode" rather than "++".
Did you think it was confusing because "next" sounded too much like the latter?
Or was it the whole "while (get more)" thing you didn't like?
Maybe it would be better to forget about using classes here and have
something like (with "bit_field_def def"):
mode = smallest_bit_field_mode (&def, params...);
while (mode != MAX_MACHINE_MODE)
{
...
mode = wider_bit_field_mode (&def, mode);
}
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-13 13:57 ` Eric Botcazou
@ 2012-11-15 12:25 ` Richard Sandiford
2012-11-15 17:10 ` Eric Botcazou
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-15 12:25 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Thanks for the reviews.
Eric Botcazou <ebotcazou@adacore.com> writes:
>> This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
>> into account, in cases where the size of the underlying object is known.
>> This is used in the next patch.
>
> Do we really need to add that to the iterator? The -fstrict-volatile-
> bitfields implementation is still controversial so I'm not sure that we want
> let it spread. Can't the client code just skip the problematic modes?
The idea was to centralise the knowledge about what modes are valid
rather than requiring every client to know the rules. From that point
of view it seems inconsistent for the new interface to handle the
bitregion_{start,end} restrictions (a correctness issue) but not the
volatility restrictions (also a correctness issue). Especially when the
interface already knows whether the field is volatile and already handles
the choice between "narrow" and "wide" volatile bitfields.
If there's talk of removing -fstrict-volatile or changing its semantics
then handling it in the interface ought to make that easier.
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-15 12:25 ` Richard Sandiford
@ 2012-11-15 17:10 ` Eric Botcazou
2012-11-15 17:47 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-15 17:10 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> The idea was to centralise the knowledge about what modes are valid
> rather than requiring every client to know the rules. From that point
> of view it seems inconsistent for the new interface to handle the
> bitregion_{start,end} restrictions (a correctness issue) but not the
> volatility restrictions (also a correctness issue). Especially when the
> interface already knows whether the field is volatile and already handles
> the choice between "narrow" and "wide" volatile bitfields.
Richard B.'s idea is precisely to reimplement -fstrict-volatile bitfields on
top of bitregion_{start,end}, that's why I'm not sure we want to make it part
of the interface at all.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-15 17:10 ` Eric Botcazou
@ 2012-11-15 17:47 ` Richard Sandiford
2012-11-15 19:32 ` Eric Botcazou
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-15 17:47 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> The idea was to centralise the knowledge about what modes are valid
>> rather than requiring every client to know the rules. From that point
>> of view it seems inconsistent for the new interface to handle the
>> bitregion_{start,end} restrictions (a correctness issue) but not the
>> volatility restrictions (also a correctness issue). Especially when the
>> interface already knows whether the field is volatile and already handles
>> the choice between "narrow" and "wide" volatile bitfields.
>
> Richard B.'s idea is precisely to reimplement -fstrict-volatile bitfields on
> top of bitregion_{start,end}, that's why I'm not sure we want to make it part
> of the interface at all.
OK. The current recursive force-mem-to-reg cases in store_bit_field_1
and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
so this patch was trying to fix what seemed like an oversight. Is it OK
to leave the code as-is (not handling -fstrict-volatile-bitfields),
or do I need to add new code to the expmed.c routines?
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-15 17:47 ` Richard Sandiford
@ 2012-11-15 19:32 ` Eric Botcazou
2012-11-18 17:36 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-15 19:32 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> OK. The current recursive force-mem-to-reg cases in store_bit_field_1
> and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
> so this patch was trying to fix what seemed like an oversight. Is it OK
> to leave the code as-is (not handling -fstrict-volatile-bitfields),
> or do I need to add new code to the expmed.c routines?
The former, I think.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-15 12:11 ` Richard Sandiford
@ 2012-11-15 20:39 ` Richard Henderson
2012-11-18 17:34 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Richard Henderson @ 2012-11-15 20:39 UTC (permalink / raw)
To: Eric Botcazou, gcc-patches, rdsandiford
On 11/15/2012 04:10 AM, Richard Sandiford wrote:
> "next" was supposed to be "find and return another mode" rather than "++".
> Did you think it was confusing because "next" sounded too much like the latter?
I wasn't keen on "next" being find-and-return, though I didn't
actually find it confusing. And perhaps rather than bikeshed
this too much now, we should table this for revision in 4.9...
> I hadn't thought about an operator bool terminator. I agree that's
> probably simpler, but do any libstdc++ classes have the same thing?
> It doesn't feel any more standard than the "while (get_more)" idiom to me,
> but that's probably just my ignorance of C++.
... when we can attack all the iterators.
No, you're right that operator bool as a terminator isn't standard.
Though for many purposes it seems better than the "!= fake_end_object"
semantics that we'd have to use otherwise.
That's a discussion that we should have generally as we find our
feet with C++ in GCC.
Unless Eric has any strong objections, I think this patch is ok.
And thus the entire patch set, as I havn't seen anything else that
raises a red flag.
r~
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-15 20:39 ` Richard Henderson
@ 2012-11-18 17:34 ` Richard Sandiford
0 siblings, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-18 17:34 UTC (permalink / raw)
To: Richard Henderson; +Cc: Eric Botcazou, gcc-patches
Richard Henderson <rth@redhat.com> writes:
> On 11/15/2012 04:10 AM, Richard Sandiford wrote:
>> "next" was supposed to be "find and return another mode" rather than "++".
>> Did you think it was confusing because "next" sounded too much like
>> the latter?
>
> I wasn't keen on "next" being find-and-return, though I didn't
> actually find it confusing. And perhaps rather than bikeshed
> this too much now, we should table this for revision in 4.9...
>
>> I hadn't thought about an operator bool terminator. I agree that's
>> probably simpler, but do any libstdc++ classes have the same thing?
>> It doesn't feel any more standard than the "while (get_more)" idiom to me,
>> but that's probably just my ignorance of C++.
>
> ... when we can attack all the iterators.
>
> No, you're right that operator bool as a terminator isn't standard.
> Though for many purposes it seems better than the "!= fake_end_object"
> semantics that we'd have to use otherwise.
>
> That's a discussion that we should have generally as we find our
> feet with C++ in GCC.
>
> Unless Eric has any strong objections, I think this patch is ok.
> And thus the entire patch set, as I havn't seen anything else that
> raises a red flag.
Thanks. Committed with the changes Eric asked for after retesting
on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [4/8] Add bit_field_mode_iterator
2012-11-13 12:44 ` Eric Botcazou
2012-11-13 21:46 ` Richard Henderson
@ 2012-11-18 17:36 ` Richard Sandiford
1 sibling, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-18 17:36 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> get_best_mode has various checks to decide what counts as an acceptable
>> bitfield mode. It actually has two copies of them, with slightly different
>> alignment checks:
>>
>> MIN (unit, BIGGEST_ALIGNMENT) > align
>>
>> vs.
>>
>> unit <= MIN (align, BIGGEST_ALIGNMENT)
>>
>> The second looks more correct, since we can't necessarily guarantee
>> larger alignments than BIGGEST_ALIGNMENT in all cases.
>
> Under the assumption that integer modes really require maximal
> alignment, i.e. whatever BIGGEST_ALIGNMENT is, I agree.
>
>> This patch adds a new iterator class that can be used to walk through
>> the modes, and rewrites get_best_mode to use it. I kept the existing
>> checks with two changes:
>>
>> - bitregion_start is now tested independently of bitregion_end
>
> The comments needs to be updated then.
>
>> + volatilep_ (volatilep), count_ (0)
>> +{
>> + if (bitregion_end_)
>> + bitregion_end_ += 1;
>> +}
>
> IMO this is confusing. I think bitregion_end/bitregion_end_ should have a
> consistent meaning.
>
>> +/* Calls to this function return successively larger modes that can be used
>> + to represent the bitfield. Return true if another bitfield mode is +
>> available, storing it in *OUT_MODE if so. */
>> +
>> +bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)
>
> 'bool' on its own line I think.
Thanks. Here's the version I committed.
Richard
gcc/
* machmode.h (bit_field_mode_iterator): New class.
(get_best_mode): Change final parameter to bool.
* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
(bit_field_mode_iterator::next_mode): New functions, split out from...
(get_best_mode): ...here. Change final parameter to bool.
Use bit_field_mode_iterator.
Index: gcc/machmode.h
===================================================================
--- gcc/machmode.h 2012-11-18 17:22:01.515895170 +0000
+++ gcc/machmode.h 2012-11-18 17:22:43.313844317 +0000
@@ -259,13 +259,36 @@ extern enum machine_mode int_mode_for_mo
extern enum machine_mode mode_for_vector (enum machine_mode, unsigned);
+/* A class for iterating through possible bitfield modes. */
+class bit_field_mode_iterator
+{
+public:
+ bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT,
+ HOST_WIDE_INT, HOST_WIDE_INT,
+ unsigned int, bool);
+ bool next_mode (enum machine_mode *);
+ bool prefer_smaller_modes ();
+
+private:
+ enum machine_mode mode_;
+ /* We use signed values here because the bit position can be negative
+ for invalid input such as gcc.dg/pr48335-8.c. */
+ HOST_WIDE_INT bitsize_;
+ HOST_WIDE_INT bitpos_;
+ HOST_WIDE_INT bitregion_start_;
+ HOST_WIDE_INT bitregion_end_;
+ unsigned int align_;
+ bool volatilep_;
+ int count_;
+};
+
/* Find the best mode to use to access a bit field. */
extern enum machine_mode get_best_mode (int, int,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned int,
- enum machine_mode, int);
+ enum machine_mode, bool);
/* Determine alignment, 1<=result<=BIGGEST_ALIGNMENT. */
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-18 17:22:37.791271752 +0000
+++ gcc/stor-layout.c 2012-11-18 17:26:31.159273478 +0000
@@ -2624,14 +2624,103 @@ fixup_unsigned_type (tree type)
layout_type (type);
}
\f
+/* Construct an iterator for a bitfield that spans BITSIZE bits,
+ starting at BITPOS.
+
+ BITREGION_START is the bit position of the first bit in this
+ sequence of bit fields. BITREGION_END is the last bit in this
+ sequence. If these two fields are non-zero, we should restrict the
+ memory access to that range. Otherwise, we are allowed to touch
+ any adjacent non bit-fields.
+
+ ALIGN is the alignment of the underlying object in bits.
+ VOLATILEP says whether the bitfield is volatile. */
+
+bit_field_mode_iterator
+::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
+ HOST_WIDE_INT bitregion_start,
+ HOST_WIDE_INT bitregion_end,
+ unsigned int align, bool volatilep)
+: mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize),
+ bitpos_ (bitpos), bitregion_start_ (bitregion_start),
+ bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
+ volatilep_ (volatilep), count_ (0)
+{
+}
+
+/* Calls to this function return successively larger modes that can be used
+ to represent the bitfield. Return true if another bitfield mode is
+ available, storing it in *OUT_MODE if so. */
+
+bool
+bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)
+{
+ for (; mode_ != VOIDmode; mode_ = GET_MODE_WIDER_MODE (mode_))
+ {
+ unsigned int unit = GET_MODE_BITSIZE (mode_);
+
+ /* Skip modes that don't have full precision. */
+ if (unit != GET_MODE_PRECISION (mode_))
+ continue;
+
+ /* Skip modes that are too small. */
+ if ((bitpos_ % unit) + bitsize_ > unit)
+ continue;
+
+ /* Stop if the mode is too wide to handle efficiently. */
+ if (unit > MAX_FIXED_MODE_SIZE)
+ break;
+
+ /* Don't deliver more than one multiword mode; the smallest one
+ should be used. */
+ if (count_ > 0 && unit > BITS_PER_WORD)
+ break;
+
+ /* Stop if the mode is wider than the alignment of the containing
+ object.
+
+ It is tempting to omit the following line unless STRICT_ALIGNMENT
+ is true. But that is incorrect, since if the bitfield uses part
+ of 3 bytes and we use a 4-byte mode, we could get a spurious segv
+ if the extra 4th byte is past the end of memory.
+ (Though at least one Unix compiler ignores this problem:
+ that on the Sequent 386 machine. */
+ if (unit > align_)
+ break;
+
+ /* Stop if the mode goes outside the bitregion. */
+ HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
+ if (bitregion_start_ && start < bitregion_start_)
+ break;
+ if (bitregion_end_ && start + unit > bitregion_end_ + 1)
+ break;
+
+ *out_mode = mode_;
+ mode_ = GET_MODE_WIDER_MODE (mode_);
+ count_++;
+ return true;
+ }
+ return false;
+}
+
+/* Return true if smaller modes are generally preferred for this kind
+ of bitfield. */
+
+bool
+bit_field_mode_iterator::prefer_smaller_modes ()
+{
+ return (volatilep_
+ ? targetm.narrow_volatile_bitfield ()
+ : !SLOW_BYTE_ACCESS);
+}
+
/* Find the best machine mode to use when referencing a bit field of length
BITSIZE bits starting at BITPOS.
BITREGION_START is the bit position of the first bit in this
sequence of bit fields. BITREGION_END is the last bit in this
sequence. If these two fields are non-zero, we should restrict the
- memory access to a maximum sized chunk of
- BITREGION_END - BITREGION_START + 1. Otherwise, we are allowed to touch
+ memory access to that range. Otherwise, we are allowed to touch
any adjacent non bit-fields.
The underlying object is known to be aligned to a boundary of ALIGN bits.
@@ -2655,69 +2744,21 @@ get_best_mode (int bitsize, int bitpos,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
unsigned int align,
- enum machine_mode largest_mode, int volatilep)
+ enum machine_mode largest_mode, bool volatilep)
{
+ bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start,
+ bitregion_end, align, volatilep);
+ enum machine_mode widest_mode = VOIDmode;
enum machine_mode mode;
- unsigned int unit = 0;
- unsigned HOST_WIDE_INT maxbits;
-
- /* If unset, no restriction. */
- if (!bitregion_end)
- maxbits = MAX_FIXED_MODE_SIZE;
- else
- maxbits = bitregion_end - bitregion_start + 1;
-
- /* Find the narrowest integer mode that contains the bit field. */
- for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
- mode = GET_MODE_WIDER_MODE (mode))
+ while (iter.next_mode (&mode)
+ && (largest_mode == VOIDmode
+ || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
{
- unit = GET_MODE_BITSIZE (mode);
- if (unit == GET_MODE_PRECISION (mode)
- && (bitpos % unit) + bitsize <= unit)
+ widest_mode = mode;
+ if (iter.prefer_smaller_modes ())
break;
}
-
- if (mode == VOIDmode
- /* It is tempting to omit the following line
- if STRICT_ALIGNMENT is true.
- But that is incorrect, since if the bitfield uses part of 3 bytes
- and we use a 4-byte mode, we could get a spurious segv
- if the extra 4th byte is past the end of memory.
- (Though at least one Unix compiler ignores this problem:
- that on the Sequent 386 machine. */
- || MIN (unit, BIGGEST_ALIGNMENT) > align
- || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE (largest_mode))
- || unit > maxbits
- || (bitregion_end
- && bitpos - (bitpos % unit) + unit > bitregion_end + 1))
- return VOIDmode;
-
- if ((SLOW_BYTE_ACCESS && ! volatilep)
- || (volatilep && !targetm.narrow_volatile_bitfield ()))
- {
- enum machine_mode wide_mode = VOIDmode, tmode;
-
- for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT); tmode != VOIDmode;
- tmode = GET_MODE_WIDER_MODE (tmode))
- {
- unit = GET_MODE_BITSIZE (tmode);
- if (unit == GET_MODE_PRECISION (tmode)
- && bitpos / unit == (bitpos + bitsize - 1) / unit
- && unit <= BITS_PER_WORD
- && unit <= MIN (align, BIGGEST_ALIGNMENT)
- && unit <= maxbits
- && (largest_mode == VOIDmode
- || unit <= GET_MODE_BITSIZE (largest_mode))
- && (bitregion_end == 0
- || bitpos - (bitpos % unit) + unit <= bitregion_end + 1))
- wide_mode = tmode;
- }
-
- if (wide_mode != VOIDmode)
- return wide_mode;
- }
-
- return mode;
+ return widest_mode;
}
/* Gets minimal and maximal values for MODE (signed or unsigned depending on
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-13 13:52 ` Eric Botcazou
@ 2012-11-18 17:36 ` Richard Sandiford
2012-11-20 2:57 ` John David Anglin
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-18 17:36 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> This patch replaces:
>>
>> /* Stop if the mode is wider than the alignment of the containing
>> object.
>>
>> It is tempting to omit the following line unless STRICT_ALIGNMENT
>> is true. But that is incorrect, since if the bitfield uses part
>> of 3 bytes and we use a 4-byte mode, we could get a spurious segv
>> if the extra 4th byte is past the end of memory.
>> (Though at least one Unix compiler ignores this problem:
>> that on the Sequent 386 machine. */
>> if (unit > align_)
>> break;
>>
>> with two checks: one for whether the final byte of the mode is known
>> to be mapped, and one for whether the bitfield is sufficiently aligned.
>> Later patches in the series rely on this in order not to pessimise
>> memory handling.
>>
>> However, as described in the patch, I found that extending this
>> behaviour to get_best_mode affected code generation for x86_64-linux-gnu
>> and powerpc64-linux-gnu, and led to a failure in bb-slp-5.c on both.
>> I therefore chickened out and added the original check back to
>> get_best_mode.
>>
>> I'm certainly interested in any feedback on the comment, but at the
>> same time I'd like this series to be a no-op on targets that keep
>> the traditional .md patterns. Any change to get_best_mode is probably
>> best done as a separate patch.
>
> I agree with your conservative approach here.
>
>> gcc/
>> * stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
>> Set up a default value of bitregion_end_.
>> (bit_field_mode_iterator::next_mode): Always apply bitregion_end_
>> check. Include SLOW_UNALIGNED_ACCESS in the alignment check.
>> (get_best_mode): Ignore modes that are wider than the alignment.
>
> Fine with me, modulo:
>
>> @@ -2754,6 +2753,62 @@ get_best_mode (int bitsize, int bitpos,
>> enum machine_mode widest_mode = VOIDmode;
>> enum machine_mode mode;
>> while (iter.next_mode (&mode)
>> + /* ??? For compatiblity, reject modes that are wider than the
>> + alignment. This has both advantages and disadvantages.
>
> "For compatibility" is a bit vague (compatibility with what?). I'd write "For
> historical reasons" or something along these lines.
Yeah, that's better. Here's what I committed after updating to the patch
for the inclusive bitregion_end_.
Richard
gcc/
* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
Set up a default value of bitregion_end_.
(bit_field_mode_iterator::next_mode): Always apply bitregion_end_
check. Include SLOW_UNALIGNED_ACCESS in the alignment check.
(get_best_mode): Ignore modes that are wider than the alignment.
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-18 17:26:31.159273478 +0000
+++ gcc/stor-layout.c 2012-11-18 17:28:17.481177253 +0000
@@ -2646,6 +2646,13 @@ fixup_unsigned_type (tree type)
bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
volatilep_ (volatilep), count_ (0)
{
+ if (!bitregion_end_)
+ {
+ /* We can assume that any aligned chunk of ALIGN_ bits that overlaps
+ the bitfield is mapped and won't trap. */
+ bitregion_end_ = bitpos + bitsize + align_ - 1;
+ bitregion_end_ -= bitregion_end_ % align_ + 1;
+ }
}
/* Calls to this function return successively larger modes that can be used
@@ -2676,23 +2683,15 @@ bit_field_mode_iterator::next_mode (enum
if (count_ > 0 && unit > BITS_PER_WORD)
break;
- /* Stop if the mode is wider than the alignment of the containing
- object.
-
- It is tempting to omit the following line unless STRICT_ALIGNMENT
- is true. But that is incorrect, since if the bitfield uses part
- of 3 bytes and we use a 4-byte mode, we could get a spurious segv
- if the extra 4th byte is past the end of memory.
- (Though at least one Unix compiler ignores this problem:
- that on the Sequent 386 machine. */
- if (unit > align_)
- break;
-
/* Stop if the mode goes outside the bitregion. */
HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
if (bitregion_start_ && start < bitregion_start_)
break;
- if (bitregion_end_ && start + unit > bitregion_end_ + 1)
+ if (start + unit > bitregion_end_ + 1)
+ break;
+
+ /* Stop if the mode requires too much alignment. */
+ if (unit > align_ && SLOW_UNALIGNED_ACCESS (mode_, align_))
break;
*out_mode = mode_;
@@ -2751,6 +2750,62 @@ get_best_mode (int bitsize, int bitpos,
enum machine_mode widest_mode = VOIDmode;
enum machine_mode mode;
while (iter.next_mode (&mode)
+ /* ??? For historical reasons, reject modes that are wider than
+ the alignment. This has both advantages and disadvantages.
+ Removing this check means that something like:
+
+ struct s { unsigned int x; unsigned int y; };
+ int f (struct s *s) { return s->x == 0 && s->y == 0; }
+
+ can be implemented using a single load and compare on
+ 64-bit machines that have no alignment restrictions.
+ For example, on powerpc64-linux-gnu, we would generate:
+
+ ld 3,0(3)
+ cntlzd 3,3
+ srdi 3,3,6
+ blr
+
+ rather than:
+
+ lwz 9,0(3)
+ cmpwi 7,9,0
+ bne 7,.L3
+ lwz 3,4(3)
+ cntlzw 3,3
+ srwi 3,3,5
+ extsw 3,3
+ blr
+ .p2align 4,,15
+ .L3:
+ li 3,0
+ blr
+
+ However, accessing more than one field can make life harder
+ for the gimple optimizers. For example, gcc.dg/vect/bb-slp-5.c
+ has a series of unsigned short copies followed by a series of
+ unsigned short comparisons. With this check, both the copies
+ and comparisons remain 16-bit accesses and FRE is able
+ to eliminate the latter. Without the check, the comparisons
+ can be done using 2 64-bit operations, which FRE isn't able
+ to handle in the same way.
+
+ Either way, it would probably be worth disabling this check
+ during expand. One particular example where removing the
+ check would help is the get_best_mode call in store_bit_field.
+ If we are given a memory bitregion of 128 bits that is aligned
+ to a 64-bit boundary, and the bitfield we want to modify is
+ in the second half of the bitregion, this check causes
+ store_bitfield to turn the memory into a 64-bit reference
+ to the _first_ half of the region. We later use
+ adjust_bitfield_address to get a reference to the correct half,
+ but doing so looks to adjust_bitfield_address as though we are
+ moving past the end of the original object, so it drops the
+ associated MEM_EXPR and MEM_OFFSET. Removing the check
+ causes store_bit_field to keep a 128-bit memory reference,
+ so that the final bitfield reference still has a MEM_EXPR
+ and MEM_OFFSET. */
+ && GET_MODE_BITSIZE (mode) <= align
&& (largest_mode == VOIDmode
|| GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
{
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [6/8] Add strict volatile handling to bit_field_mode_iterator
2012-11-15 19:32 ` Eric Botcazou
@ 2012-11-18 17:36 ` Richard Sandiford
0 siblings, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-18 17:36 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> OK. The current recursive force-mem-to-reg cases in store_bit_field_1
>> and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
>> so this patch was trying to fix what seemed like an oversight. Is it OK
>> to leave the code as-is (not handling -fstrict-volatile-bitfields),
>> or do I need to add new code to the expmed.c routines?
>
> The former, I think.
OK, I left this patch out and removed the associated constructor argument
from patch 7.
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-18 17:36 ` Richard Sandiford
@ 2012-11-20 2:57 ` John David Anglin
2012-11-20 8:21 ` Mikael Pettersson
0 siblings, 1 reply; 42+ messages in thread
From: John David Anglin @ 2012-11-20 2:57 UTC (permalink / raw)
To: Eric Botcazou, gcc-patches, rdsandiford
On Sun, 18 Nov 2012, Richard Sandiford wrote:
> HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
> if (bitregion_start_ && start < bitregion_start_)
> break;
> - if (bitregion_end_ && start + unit > bitregion_end_ + 1)
> + if (start + unit > bitregion_end_ + 1)
This causes:
/home/dave/gnu/gcc/objdir/./prev-gcc/g++ -B/home/dave/gnu/gcc/objdir/./prev-gcc/ -B/home/dave/opt/gnu/gcc/gcc-4.8.0/hppa-linux-gnu/bin/ -nostdinc++ -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/src/.libs -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/include/hppa-linux-gnu -I/home/dave/gnu/gcc
/objdir/prev-hppa-linux-gnu/libstdc++-v3/include -I/home/dave/gnu/gcc/gcc/libstdc++-v3/libsupc++ -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/sr
c/.libs -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -c -g -O2 -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tabl
es -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribut
e -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror
-fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc/gcc -I../../gcc/gcc/. -I../../g
cc/gcc/../include -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libdecnu
mber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/gcc/../li
bbacktrace ../../gcc/gcc/stor-layout.c -o stor-layout.o../../gcc/gcc/stor-layout.c: In member function 〘bool bit_field_mode_iterator::n
ext_mode(machine_mode*)〙:
../../gcc/gcc/stor-layout.c:2690:43: error: comparison between signed and unsign
ed integer expressions [-Werror=sign-compare]
if (start + unit > bitregion_end_ + 1)
^
cc1plus: all warnings being treated as errors
Dave
--
J. David Anglin dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-20 2:57 ` John David Anglin
@ 2012-11-20 8:21 ` Mikael Pettersson
2012-11-20 10:32 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Mikael Pettersson @ 2012-11-20 8:21 UTC (permalink / raw)
To: John David Anglin; +Cc: Eric Botcazou, gcc-patches, rdsandiford
John David Anglin writes:
> On Sun, 18 Nov 2012, Richard Sandiford wrote:
>
> > HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
> > if (bitregion_start_ && start < bitregion_start_)
> > break;
> > - if (bitregion_end_ && start + unit > bitregion_end_ + 1)
> > + if (start + unit > bitregion_end_ + 1)
>
> This causes:
>
> /home/dave/gnu/gcc/objdir/./prev-gcc/g++ -B/home/dave/gnu/gcc/objdir/./prev-gcc/ -B/home/dave/opt/gnu/gcc/gcc-4.8.0/hppa-linux-gnu/bin/ -nostdinc++ -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/src/.libs -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/include/hppa-linux-gnu -I/home/dave/gnu/gcc
> /objdir/prev-hppa-linux-gnu/libstdc++-v3/include -I/home/dave/gnu/gcc/gcc/libstdc++-v3/libsupc++ -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/sr
> c/.libs -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -c -g -O2 -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tabl
> es -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribut
> e -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror
> -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc/gcc -I../../gcc/gcc/. -I../../g
> cc/gcc/../include -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libdecnu
> mber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/gcc/../li
> bbacktrace ../../gcc/gcc/stor-layout.c -o stor-layout.o../../gcc/gcc/stor-layout.c: In member function 〘bool bit_field_mode_iterator::n
> ext_mode(machine_mode*)〙:
> ../../gcc/gcc/stor-layout.c:2690:43: error: comparison between signed and unsign
> ed integer expressions [-Werror=sign-compare]
> if (start + unit > bitregion_end_ + 1)
> ^
> cc1plus: all warnings being treated as errors
This error also breaks m68k-linux bootstrap.
HWI32 issue?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-20 8:21 ` Mikael Pettersson
@ 2012-11-20 10:32 ` Richard Sandiford
2012-11-20 19:56 ` Richard Sandiford
2012-11-20 22:11 ` Eric Botcazou
0 siblings, 2 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-20 10:32 UTC (permalink / raw)
To: Mikael Pettersson; +Cc: John David Anglin, Eric Botcazou, gcc-patches
Mikael Pettersson <mikpe@it.uu.se> writes:
> John David Anglin writes:
> > On Sun, 18 Nov 2012, Richard Sandiford wrote:
> >
> > > HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
> > > if (bitregion_start_ && start < bitregion_start_)
> > > break;
> > > - if (bitregion_end_ && start + unit > bitregion_end_ + 1)
> > > + if (start + unit > bitregion_end_ + 1)
> >
> > This causes:
> >
> > /home/dave/gnu/gcc/objdir/./prev-gcc/g++ -B/home/dave/gnu/gcc/objdir/./prev-gcc/ -B/home/dave/opt/gnu/gcc/gcc-4.8.0/hppa-linux-gnu/bin/ -nostdinc++ -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/src/.libs -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/include/hppa-linux-gnu -I/home/dave/gnu/gcc
> > /objdir/prev-hppa-linux-gnu/libstdc++-v3/include -I/home/dave/gnu/gcc/gcc/libstdc++-v3/libsupc++ -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/sr
> > c/.libs -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -c -g -O2 -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tabl
> > es -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribut
> > e -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror
> > -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc/gcc -I../../gcc/gcc/. -I../../g
> > cc/gcc/../include -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libdecnu
> > mber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/gcc/../li
> > bbacktrace ../../gcc/gcc/stor-layout.c -o stor-layout.o../../gcc/gcc/stor-layout.c: In member function 〘bool bit_field_mode_iterator::n
> > ext_mode(machine_mode*)〙:
> > ../../gcc/gcc/stor-layout.c:2690:43: error: comparison between signed and unsign
> > ed integer expressions [-Werror=sign-compare]
> > if (start + unit > bitregion_end_ + 1)
> > ^
> > cc1plus: all warnings being treated as errors
>
> This error also breaks m68k-linux bootstrap.
>
> HWI32 issue?
Yeah, I expect so, sorry.
Logically, everything here would be unsigned arithmetic, but as the
comment says:
/* We use signed values here because the bit position can be negative
for invalid input such as gcc.dg/pr48335-8.c. */
This is the patch I'm testing. There are three things being checked here:
- "unit", the size of the mode in isolation. This really is an unsigned
value, and is compared to unsigned values like GET_MODE_PRECISION.
- bitpos_ % unit (+ bitsize_), the start and end positions of the bitfield
relative to the start of the mode. The start position is supposed to be
[0, unit), so the modulus and result should be unsigned. (Using unsigned
modulus doesn't cope with negative bit positions combined with
non-power-of-2 units, but I don't think we support that.)
- bitregion_start_ and bitregion_end_. bitpos_ is signed and can be
negative, so the bitregion comparison should continue to be signed.
OK to commit if testing succeeds?
Richard
gcc/
* stor-layout.c (bit_field_mode_iterator::next_mode): Fix signedness.
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c 2012-11-20 10:15:39.000000000 +0000
+++ gcc/stor-layout.c 2012-11-20 10:15:39.464712715 +0000
@@ -2670,10 +2670,6 @@ bit_field_mode_iterator::next_mode (enum
if (unit != GET_MODE_PRECISION (mode_))
continue;
- /* Skip modes that are too small. */
- if ((bitpos_ % unit) + bitsize_ > unit)
- continue;
-
/* Stop if the mode is too wide to handle efficiently. */
if (unit > MAX_FIXED_MODE_SIZE)
break;
@@ -2683,11 +2679,18 @@ bit_field_mode_iterator::next_mode (enum
if (count_ > 0 && unit > BITS_PER_WORD)
break;
+ /* Skip modes that are too small. */
+ unsigned HOST_WIDE_INT substart = (unsigned HOST_WIDE_INT) bitpos_ % unit;
+ unsigned HOST_WIDE_INT subend = substart + bitsize_;
+ if (subend > unit)
+ continue;
+
/* Stop if the mode goes outside the bitregion. */
- HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
+ HOST_WIDE_INT start = bitpos_ - substart;
if (bitregion_start_ && start < bitregion_start_)
break;
- if (start + unit > bitregion_end_ + 1)
+ HOST_WIDE_INT end = start + unit;
+ if (end > bitregion_end_ + 1)
break;
/* Stop if the mode requires too much alignment. */
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-20 10:32 ` Richard Sandiford
@ 2012-11-20 19:56 ` Richard Sandiford
2012-11-20 22:11 ` Eric Botcazou
1 sibling, 0 replies; 42+ messages in thread
From: Richard Sandiford @ 2012-11-20 19:56 UTC (permalink / raw)
To: Mikael Pettersson; +Cc: John David Anglin, Eric Botcazou, gcc-patches
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Mikael Pettersson <mikpe@it.uu.se> writes:
>> John David Anglin writes:
>> > On Sun, 18 Nov 2012, Richard Sandiford wrote:
>> >
>> > > HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
>> > > if (bitregion_start_ && start < bitregion_start_)
>> > > break;
>> > > - if (bitregion_end_ && start + unit > bitregion_end_ + 1)
>> > > + if (start + unit > bitregion_end_ + 1)
>> >
>> > This causes:
>> >
>> > /home/dave/gnu/gcc/objdir/./prev-gcc/g++ -B/home/dave/gnu/gcc/objdir/./prev-gcc/ -B/home/dave/opt/gnu/gcc/gcc-4.8.0/hppa-linux-gnu/bin/ -nostdinc++ -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/src/.libs -B/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/include/hppa-linux-gnu -I/home/dave/gnu/gcc
>> > /objdir/prev-hppa-linux-gnu/libstdc++-v3/include -I/home/dave/gnu/gcc/gcc/libstdc++-v3/libsupc++ -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/sr
>> > c/.libs -L/home/dave/gnu/gcc/objdir/prev-hppa-linux-gnu/libstdc++-v3/libsupc++/.libs -c -g -O2 -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tabl
>> > es -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribut
>> > e -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror
>> > -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc/gcc -I../../gcc/gcc/. -I../../g
>> > cc/gcc/../include -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libdecnu
>> > mber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/gcc/../li
>> > bbacktrace ../../gcc/gcc/stor-layout.c -o stor-layout.o../../gcc/gcc/stor-layout.c: In member function 〘bool bit_field_mode_iterator::n
>> > ext_mode(machine_mode*)〙:
>> > ../../gcc/gcc/stor-layout.c:2690:43: error: comparison between signed and unsign
>> > ed integer expressions [-Werror=sign-compare]
>> > if (start + unit > bitregion_end_ + 1)
>> > ^
>> > cc1plus: all warnings being treated as errors
>>
>> This error also breaks m68k-linux bootstrap.
>>
>> HWI32 issue?
>
> Yeah, I expect so, sorry.
>
> Logically, everything here would be unsigned arithmetic, but as the
> comment says:
>
> /* We use signed values here because the bit position can be negative
> for invalid input such as gcc.dg/pr48335-8.c. */
>
> This is the patch I'm testing. There are three things being checked here:
>
> - "unit", the size of the mode in isolation. This really is an unsigned
> value, and is compared to unsigned values like GET_MODE_PRECISION.
>
> - bitpos_ % unit (+ bitsize_), the start and end positions of the bitfield
> relative to the start of the mode. The start position is supposed to be
> [0, unit), so the modulus and result should be unsigned. (Using unsigned
> modulus doesn't cope with negative bit positions combined with
> non-power-of-2 units, but I don't think we support that.)
>
> - bitregion_start_ and bitregion_end_. bitpos_ is signed and can be
> negative, so the bitregion comparison should continue to be signed.
>
> OK to commit if testing succeeds?
Now bootstrapped & regression-tested on x86_64-linux-gnu.
>
> Richard
>
>
> gcc/
> * stor-layout.c (bit_field_mode_iterator::next_mode): Fix signedness.
>
> Index: gcc/stor-layout.c
> ===================================================================
> --- gcc/stor-layout.c 2012-11-20 10:15:39.000000000 +0000
> +++ gcc/stor-layout.c 2012-11-20 10:15:39.464712715 +0000
> @@ -2670,10 +2670,6 @@ bit_field_mode_iterator::next_mode (enum
> if (unit != GET_MODE_PRECISION (mode_))
> continue;
>
> - /* Skip modes that are too small. */
> - if ((bitpos_ % unit) + bitsize_ > unit)
> - continue;
> -
> /* Stop if the mode is too wide to handle efficiently. */
> if (unit > MAX_FIXED_MODE_SIZE)
> break;
> @@ -2683,11 +2679,18 @@ bit_field_mode_iterator::next_mode (enum
> if (count_ > 0 && unit > BITS_PER_WORD)
> break;
>
> + /* Skip modes that are too small. */
> + unsigned HOST_WIDE_INT substart = (unsigned HOST_WIDE_INT) bitpos_ % unit;
> + unsigned HOST_WIDE_INT subend = substart + bitsize_;
> + if (subend > unit)
> + continue;
> +
> /* Stop if the mode goes outside the bitregion. */
> - HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
> + HOST_WIDE_INT start = bitpos_ - substart;
> if (bitregion_start_ && start < bitregion_start_)
> break;
> - if (start + unit > bitregion_end_ + 1)
> + HOST_WIDE_INT end = start + unit;
> + if (end > bitregion_end_ + 1)
> break;
>
> /* Stop if the mode requires too much alignment. */
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [5/8] Tweak bitfield alignment handling
2012-11-20 10:32 ` Richard Sandiford
2012-11-20 19:56 ` Richard Sandiford
@ 2012-11-20 22:11 ` Eric Botcazou
1 sibling, 0 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-20 22:11 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches, Mikael Pettersson, John David Anglin
> gcc/
> * stor-layout.c (bit_field_mode_iterator::next_mode): Fix signedness.
This looks fine to me.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
` (7 preceding siblings ...)
2012-11-03 11:41 ` [8/8] Add new optabs and use them for MIPS Richard Sandiford
@ 2012-11-27 17:11 ` Ramana Radhakrishnan
2012-11-27 20:22 ` Richard Sandiford
8 siblings, 1 reply; 42+ messages in thread
From: Ramana Radhakrishnan @ 2012-11-27 17:11 UTC (permalink / raw)
To: rdsandiford; +Cc: gcc-patches
> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
> Also tested by making sure that there were no changes in assembly
> output for a set of gcc .ii files. On the other hand, the -march=octeon
> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
> kicking in as hoped.
This sequence of patches caused a regression in
gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
stack in great detail yet, but it looks like this sequence of patches
doesn't take the ARM volatile bitfields into account. (193600 is fine,
193606 is not).
regards,
Ramana
>
> Richard
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-27 17:11 ` [0/8] Add optabs alternatives for insv, extv and extzv Ramana Radhakrishnan
@ 2012-11-27 20:22 ` Richard Sandiford
2012-11-27 22:45 ` Ramana Radhakrishnan
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-27 20:22 UTC (permalink / raw)
To: ramrad01; +Cc: gcc-patches
Ramana Radhakrishnan <ramrad01@arm.com> writes:
>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>> Also tested by making sure that there were no changes in assembly
>> output for a set of gcc .ii files. On the other hand, the -march=octeon
>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>> kicking in as hoped.
>
> This sequence of patches caused a regression in
> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
> stack in great detail yet, but it looks like this sequence of patches
> doesn't take the ARM volatile bitfields into account. (193600 is fine,
> 193606 is not).
Looks like I was wrong to drop the conditions from patch 5.
Please could you try the attached?
Richard
gcc/
* expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
bitfields.
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000
+++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000
@@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra
bitregion_end, MEM_ALIGN (op0),
MEM_VOLATILE_P (op0));
enum machine_mode best_mode;
+ if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
+ {
+ while (iter.next_mode (&best_mode))
+ if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
+ return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
+ new_bitnum);
+ return NULL_RTX;
+ }
+
if (iter.next_mode (&best_mode))
{
/* We can use a memory in BEST_MODE. See whether this is true for
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-27 20:22 ` Richard Sandiford
@ 2012-11-27 22:45 ` Ramana Radhakrishnan
2012-11-28 10:25 ` Richard Biener
2012-11-28 13:58 ` Richard Sandiford
0 siblings, 2 replies; 42+ messages in thread
From: Ramana Radhakrishnan @ 2012-11-27 22:45 UTC (permalink / raw)
To: ramrad01, gcc-patches, rdsandiford
On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>> Also tested by making sure that there were no changes in assembly
>>> output for a set of gcc .ii files. On the other hand, the -march=octeon
>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>> kicking in as hoped.
>>
>> This sequence of patches caused a regression in
>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>> stack in great detail yet, but it looks like this sequence of patches
>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>> 193606 is not).
>
> Looks like I was wrong to drop the conditions from patch 5.
> Please could you try the attached?
Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .
Ramana
>
> Richard
>
>
> gcc/
> * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
> bitfields.
>
> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000
> +++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000
> @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra
> bitregion_end, MEM_ALIGN (op0),
> MEM_VOLATILE_P (op0));
> enum machine_mode best_mode;
> + if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
> + {
> + while (iter.next_mode (&best_mode))
> + if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
> + return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
> + new_bitnum);
> + return NULL_RTX;
> + }
> +
> if (iter.next_mode (&best_mode))
> {
> /* We can use a memory in BEST_MODE. See whether this is true for
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-27 22:45 ` Ramana Radhakrishnan
@ 2012-11-28 10:25 ` Richard Biener
2012-11-28 12:06 ` Ramana Radhakrishnan
2012-11-28 13:58 ` Richard Sandiford
1 sibling, 1 reply; 42+ messages in thread
From: Richard Biener @ 2012-11-28 10:25 UTC (permalink / raw)
To: ramrad01; +Cc: gcc-patches, rdsandiford
On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>> Also tested by making sure that there were no changes in assembly
>>>> output for a set of gcc .ii files. On the other hand, the -march=octeon
>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>> kicking in as hoped.
>>>
>>> This sequence of patches caused a regression in
>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>> stack in great detail yet, but it looks like this sequence of patches
>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>> 193606 is not).
>>
>> Looks like I was wrong to drop the conditions from patch 5.
>> Please could you try the attached?
>
> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .
Can arm folks please re-implement strict volatile bitfields in terms of
DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
Thus, adjust stor-layout.c to compute a proper representative honoring
strict volatile bitfield semantics?
Thanks,
Richard.
> Ramana
>
>>
>> Richard
>>
>>
>> gcc/
>> * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
>> bitfields.
>>
>> Index: gcc/expmed.c
>> ===================================================================
>> --- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000
>> +++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000
>> @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra
>> bitregion_end, MEM_ALIGN (op0),
>> MEM_VOLATILE_P (op0));
>> enum machine_mode best_mode;
>> + if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
>> + {
>> + while (iter.next_mode (&best_mode))
>> + if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
>> + return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
>> + new_bitnum);
>> + return NULL_RTX;
>> + }
>> +
>> if (iter.next_mode (&best_mode))
>> {
>> /* We can use a memory in BEST_MODE. See whether this is true for
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-28 10:25 ` Richard Biener
@ 2012-11-28 12:06 ` Ramana Radhakrishnan
2012-11-28 12:51 ` Richard Biener
0 siblings, 1 reply; 42+ messages in thread
From: Ramana Radhakrishnan @ 2012-11-28 12:06 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, rdsandiford
On 11/28/12 10:25, Richard Biener wrote:
> On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
> <ramana.gcc@googlemail.com> wrote:
>> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>>> Also tested by making sure that there were no changes in assembly
>>>>> output for a set of gcc .ii files. On the other hand, the -march=octeon
>>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>>> kicking in as hoped.
>>>>
>>>> This sequence of patches caused a regression in
>>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>>> stack in great detail yet, but it looks like this sequence of patches
>>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>>> 193606 is not).
>>>
>>> Looks like I was wrong to drop the conditions from patch 5.
>>> Please could you try the attached?
>>
>> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .
>
> Can arm folks please re-implement strict volatile bitfields in terms of
> DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
> Thus, adjust stor-layout.c to compute a proper representative honoring
> strict volatile bitfield semantics?
This is a regression in a feature that used to work before this patch
went in.
I've now opened PR55516 to track this and in my book this is a P1
regression in a primary port and will break the USB stack in the Linux
kernel from previous experience (PR51442)
If this has to be rewritten, are you suggesting that this be done in
time for 4.8 ? I've found the reference to your previous posts on the
subject but on a quick perusal I don't see it as an easy fix. None of us
understand the code as well as you do :)
OTOH I would have naively expected such a rewrite to be painful in
stage3 even if we worked our way around this area.
Thanks,
Ramana
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-28 12:06 ` Ramana Radhakrishnan
@ 2012-11-28 12:51 ` Richard Biener
0 siblings, 0 replies; 42+ messages in thread
From: Richard Biener @ 2012-11-28 12:51 UTC (permalink / raw)
To: ramrad01; +Cc: gcc-patches, rdsandiford
On Wed, Nov 28, 2012 at 1:06 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
> On 11/28/12 10:25, Richard Biener wrote:
>>
>> On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
>> <ramana.gcc@googlemail.com> wrote:
>>>
>>> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>>
>>>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>>>>
>>>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>>>> Also tested by making sure that there were no changes in assembly
>>>>>> output for a set of gcc .ii files. On the other hand, the
>>>>>> -march=octeon
>>>>>> output for a set of mips64-linux-gnu gcc .ii files showed the
>>>>>> optimisation
>>>>>> kicking in as hoped.
>>>>>
>>>>>
>>>>> This sequence of patches caused a regression in
>>>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>>>> stack in great detail yet, but it looks like this sequence of patches
>>>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>>>> 193606 is not).
>>>>
>>>>
>>>> Looks like I was wrong to drop the conditions from patch 5.
>>>> Please could you try the attached?
>>>
>>>
>>> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c
>>> :( .
>>
>>
>> Can arm folks please re-implement strict volatile bitfields in terms of
>> DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
>> Thus, adjust stor-layout.c to compute a proper representative honoring
>> strict volatile bitfield semantics?
>
>
> This is a regression in a feature that used to work before this patch went
> in.
>
> I've now opened PR55516 to track this and in my book this is a P1 regression
> in a primary port and will break the USB stack in the Linux kernel from
> previous experience (PR51442)
>
> If this has to be rewritten, are you suggesting that this be done in time
> for 4.8 ? I've found the reference to your previous posts on the subject but
> on a quick perusal I don't see it as an easy fix. None of us understand the
> code as well as you do :)
>
> OTOH I would have naively expected such a rewrite to be painful in stage3
> even if we worked our way around this area.
It should correspond to implementing the semantics and warnings in one
single place - finish_bitfield_layout. All the RTL expansion code can be
removed then.
As I am not familiar with the ARM strict volatile bitfield semantics I am
of no help here, but I promise to review the patch if you come up with it.
Thanks,
Richard.
> Thanks,
> Ramana
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-27 22:45 ` Ramana Radhakrishnan
2012-11-28 10:25 ` Richard Biener
@ 2012-11-28 13:58 ` Richard Sandiford
2012-11-28 23:19 ` Eric Botcazou
1 sibling, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-28 13:58 UTC (permalink / raw)
To: ramrad01; +Cc: gcc-patches
Ramana Radhakrishnan <ramana.gcc@googlemail.com> writes:
> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>> Also tested by making sure that there were no changes in assembly
>>>> output for a set of gcc .ii files. On the other hand, the -march=octeon
>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>> kicking in as hoped.
>>>
>>> This sequence of patches caused a regression in
>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>> stack in great detail yet, but it looks like this sequence of patches
>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>> 193606 is not).
>>
>> Looks like I was wrong to drop the conditions from patch 5.
>> Please could you try the attached?
>
> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .
This is because the structure we are given is:
(mem/v/j:SI (reg/v/f:SI 110 [ t ]) [3 t_2(D)->a+0 S1 A32])
i.e. a 1-byte SImode reference. The strange size comes from the
set_mem_attributes_minus_bitpos code that was mentioned earlier
in the series, where we set the size based on the type even if
it doesn't match the mode.
The original rules for forcing strict-volatile bitfields from memory
to registers were different (or at least written in a different way)
between store_bit_field_1 -- where we force to a register in an attempt
to use register insvs -- and store_fixed_bit_field -- where we use the
fallback shifts and masks -- even though logically I thought they'd be
the same. In store_bit_field_1 the mode was chosen like this:
/* Get the mode to use for inserting into this field. If OP0 is
BLKmode, get the smallest mode consistent with the alignment. If
OP0 is a non-BLKmode object that is no wider than OP_MODE, use its
mode. Otherwise, use the smallest mode containing the field. */
if (GET_MODE (op0) == BLKmode
|| (op_mode != MAX_MACHINE_MODE
&& GET_MODE_SIZE (GET_MODE (op0)) > GET_MODE_SIZE (op_mode)))
bestmode = get_best_mode (bitsize, bitnum, MEM_ALIGN (op0),
(op_mode == MAX_MACHINE_MODE
? VOIDmode : op_mode),
MEM_VOLATILE_P (op0));
else
bestmode = GET_MODE (op0);
if (bestmode != VOIDmode
&& GET_MODE_SIZE (bestmode) >= GET_MODE_SIZE (fieldmode)
&& !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
&& GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
{
i.e. no explicit test of flag_strict_volatile_bitfields.
In store_fixed_bit_field we had:
mode = GET_MODE (op0);
if (GET_MODE_BITSIZE (mode) == 0
|| GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
mode = word_mode;
if (MEM_VOLATILE_P (op0)
&& GET_MODE_BITSIZE (GET_MODE (op0)) > 0
&& flag_strict_volatile_bitfields > 0)
mode = GET_MODE (op0);
else
mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
The same thing applied to extract_bit_field_1 and extract_fixed_bit_field,
with the latter using:
if (MEM_VOLATILE_P (op0)
&& flag_strict_volatile_bitfields > 0)
{
if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
mode = GET_MODE (op0);
else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
mode = GET_MODE (target);
else
mode = tmode;
}
else
mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
This patch seems to fix the volatile bitfield tests, both with the
default arm-linux-gnueabi (which already works) and with
-mcpu=cortex-a8. Could you give it a proper test?
Richard
gcc/
* expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
bitfields.
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c (revision 193881)
+++ gcc/expmed.c (working copy)
@@ -372,6 +372,17 @@
bitregion_end, MEM_ALIGN (op0),
MEM_VOLATILE_P (op0));
enum machine_mode best_mode;
+ if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
+ {
+ unsigned int size = GET_MODE_BITSIZE (GET_MODE (op0));
+ if (size > 0)
+ while (iter.next_mode (&best_mode))
+ if (GET_MODE_BITSIZE (best_mode) == size)
+ return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
+ new_bitnum);
+ return NULL_RTX;
+ }
+
if (iter.next_mode (&best_mode))
{
/* We can use a memory in BEST_MODE. See whether this is true for
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-28 13:58 ` Richard Sandiford
@ 2012-11-28 23:19 ` Eric Botcazou
2012-11-29 10:31 ` Richard Sandiford
0 siblings, 1 reply; 42+ messages in thread
From: Eric Botcazou @ 2012-11-28 23:19 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches, ramrad01
> This is because the structure we are given is:
>
> (mem/v/j:SI (reg/v/f:SI 110 [ t ]) [3 t_2(D)->a+0 S1 A32])
>
> i.e. a 1-byte SImode reference. The strange size comes from the
> set_mem_attributes_minus_bitpos code that was mentioned earlier
> in the series, where we set the size based on the type even if
> it doesn't match the mode.
I think that's wrong, we should have S4 and drop the MEM_EXPR as we would do
it with adjust_bitfield_address. In other words, if the size computed from
the tree is smaller than the mode size, we don't use it and clear MEM_EXPR.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-28 23:19 ` Eric Botcazou
@ 2012-11-29 10:31 ` Richard Sandiford
2012-11-29 15:31 ` Eric Botcazou
0 siblings, 1 reply; 42+ messages in thread
From: Richard Sandiford @ 2012-11-29 10:31 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, ramrad01
Eric Botcazou <ebotcazou@adacore.com> writes:
>> This is because the structure we are given is:
>>
>> (mem/v/j:SI (reg/v/f:SI 110 [ t ]) [3 t_2(D)->a+0 S1 A32])
>>
>> i.e. a 1-byte SImode reference. The strange size comes from the
>> set_mem_attributes_minus_bitpos code that was mentioned earlier
>> in the series, where we set the size based on the type even if
>> it doesn't match the mode.
>
> I think that's wrong, we should have S4 and drop the MEM_EXPR as we would do
> it with adjust_bitfield_address. In other words, if the size computed from
> the tree is smaller than the mode size, we don't use it and clear MEM_EXPR.
I agree that this kind of MEM is less than ideal, but I thought:
set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
said that the attributes of TO_RTX will to be TO _once a pending offset-and-
narrowing operation has been applied_. So we have:
/* If we modified OFFSET based on T, then subtract the outstanding
bit position offset. Similarly, increase the size of the accessed
object to contain the negative offset. */
if (apply_bitpos)
{
gcc_assert (attrs.offset_known_p);
attrs.offset -= apply_bitpos / BITS_PER_UNIT;
if (attrs.size_known_p)
attrs.size += apply_bitpos / BITS_PER_UNIT;
}
I didn't think we necessarily expected the width of the reference
(TO_RTX) and the width of the type (TO) to match at this stage.
That's different from adjust_bitfield_address, where the
offset-and-narrowing operation itself is applied.
The difference between the width of the reference and the width
of T is what led to:
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00262.html
As things stand, APPLY_BITPOS is only nonzero if we set both the
MEM_EXPR and MEM_SIZE from T. There are also cases (like this one)
where we don't set the MEM_EXPR from T but do set the MEM_SIZE from T.
The bitpos will be applied either way, so I thought MEM_SIZE should be
the same in both cases. That doesn't fix this problem of course, it's
just an argument that the relationship between the width of the reference
mode, the MEM_SIZE and the width of T seems pretty complicated with the
current interface.
Maybe set_mem_attributes_minus_bitpos (but not set_mem_attributes)
should only set the MEM_EXPR and leave the MEM_SIZE unchanged?
Before submitting the patched linked above, I tried getting rid
of set_mem_attributes_minus_bitpos and passing the tree down instead.
Then we could set the attributes at the time of the offset-and-narrowing
operation, where the size and offset of the final reference are known.
That didn't seem like an easy change to make though, and became a
bit of a distraction from the main patches.
Anyway, given the breakage that this series has already caused,
I'd prefer not to tackle stuff like this as well. I'd only used
MEM_SIZE in the first attempted patch out of habit. I think the
revised patch more obviously matches the *_fixed_bit_field functions
and is more generally in keeping with the existing checks.
(It's deliberately more conservative though, only using register
bitfields if both the bit_field_mode_iterator and strict volatile
bitfield rules are met.)
Richard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [0/8] Add optabs alternatives for insv, extv and extzv
2012-11-29 10:31 ` Richard Sandiford
@ 2012-11-29 15:31 ` Eric Botcazou
0 siblings, 0 replies; 42+ messages in thread
From: Eric Botcazou @ 2012-11-29 15:31 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches, ramrad01
> I agree that this kind of MEM is less than ideal, but I thought:
>
> set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
>
> said that the attributes of TO_RTX will to be TO _once a pending offset-and-
> narrowing operation has been applied_. So we have:
>
> /* If we modified OFFSET based on T, then subtract the outstanding
> bit position offset. Similarly, increase the size of the accessed
> object to contain the negative offset. */
> if (apply_bitpos)
> {
> gcc_assert (attrs.offset_known_p);
> attrs.offset -= apply_bitpos / BITS_PER_UNIT;
> if (attrs.size_known_p)
> attrs.size += apply_bitpos / BITS_PER_UNIT;
> }
>
> I didn't think we necessarily expected the width of the reference
> (TO_RTX) and the width of the type (TO) to match at this stage.
> That's different from adjust_bitfield_address, where the
> offset-and-narrowing operation itself is applied.
I was essentially thinking of the size adjustment just above that one: if the
mode size is known, setting a smaller size without further ado seems awkward.
So the questionable MEM doesn't survive long? OK, maybe...
> The difference between the width of the reference and the width
> of T is what led to:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00262.html
>
> As things stand, APPLY_BITPOS is only nonzero if we set both the
> MEM_EXPR and MEM_SIZE from T. There are also cases (like this one)
> where we don't set the MEM_EXPR from T but do set the MEM_SIZE from T.
> The bitpos will be applied either way, so I thought MEM_SIZE should be
> the same in both cases. That doesn't fix this problem of course, it's
> just an argument that the relationship between the width of the reference
> mode, the MEM_SIZE and the width of T seems pretty complicated with the
> current interface.
MEM_SIZE and MEM_EXPR are used alone by the aliasing machinery to disambiguate
memory references, so they need be conservative wrt the actual memory access.
> Maybe set_mem_attributes_minus_bitpos (but not set_mem_attributes)
> should only set the MEM_EXPR and leave the MEM_SIZE unchanged?
>
> Before submitting the patched linked above, I tried getting rid
> of set_mem_attributes_minus_bitpos and passing the tree down instead.
> Then we could set the attributes at the time of the offset-and-narrowing
> operation, where the size and offset of the final reference are known.
> That didn't seem like an easy change to make though, and became a
> bit of a distraction from the main patches.
>
> Anyway, given the breakage that this series has already caused,
> I'd prefer not to tackle stuff like this as well. I'd only used
> MEM_SIZE in the first attempted patch out of habit. I think the
> revised patch more obviously matches the *_fixed_bit_field functions
> and is more generally in keeping with the existing checks.
> (It's deliberately more conservative though, only using register
> bitfields if both the bit_field_mode_iterator and strict volatile
> bitfield rules are met.)
Well, rewriting the bitfield machinery of the middle-end is a once-in-a-decade
undertaking, so some fallouts are to be expected. :-) That wasn't too bad in
the end. But I agree with the cautious approach from now on.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2012-11-29 15:31 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-03 11:10 [0/8] Add optabs alternatives for insv, extv and extzv Richard Sandiford
2012-11-03 11:13 ` [1/8] Handle TRUNCATE in make_extraction Richard Sandiford
2012-11-10 15:52 ` Eric Botcazou
2012-11-03 11:14 ` [2/8] Add adjust_bitfield_address_size Richard Sandiford
2012-11-10 15:53 ` Eric Botcazou
2012-11-03 11:16 ` [3/8] Add narrow_bit_field_mem Richard Sandiford
2012-11-10 16:02 ` Eric Botcazou
2012-11-03 11:21 ` [4/8] Add bit_field_mode_iterator Richard Sandiford
2012-11-13 12:44 ` Eric Botcazou
2012-11-13 21:46 ` Richard Henderson
2012-11-13 22:05 ` Eric Botcazou
2012-11-15 12:11 ` Richard Sandiford
2012-11-15 20:39 ` Richard Henderson
2012-11-18 17:34 ` Richard Sandiford
2012-11-18 17:36 ` Richard Sandiford
2012-11-03 11:27 ` [5/8] Tweak bitfield alignment handling Richard Sandiford
2012-11-13 13:52 ` Eric Botcazou
2012-11-18 17:36 ` Richard Sandiford
2012-11-20 2:57 ` John David Anglin
2012-11-20 8:21 ` Mikael Pettersson
2012-11-20 10:32 ` Richard Sandiford
2012-11-20 19:56 ` Richard Sandiford
2012-11-20 22:11 ` Eric Botcazou
2012-11-03 11:28 ` [6/8] Add strict volatile handling to bit_field_mode_iterator Richard Sandiford
2012-11-13 13:57 ` Eric Botcazou
2012-11-15 12:25 ` Richard Sandiford
2012-11-15 17:10 ` Eric Botcazou
2012-11-15 17:47 ` Richard Sandiford
2012-11-15 19:32 ` Eric Botcazou
2012-11-18 17:36 ` Richard Sandiford
2012-11-03 11:39 ` [7/8] Replace mode_for_extraction with new interface Richard Sandiford
2012-11-03 11:41 ` [8/8] Add new optabs and use them for MIPS Richard Sandiford
2012-11-27 17:11 ` [0/8] Add optabs alternatives for insv, extv and extzv Ramana Radhakrishnan
2012-11-27 20:22 ` Richard Sandiford
2012-11-27 22:45 ` Ramana Radhakrishnan
2012-11-28 10:25 ` Richard Biener
2012-11-28 12:06 ` Ramana Radhakrishnan
2012-11-28 12:51 ` Richard Biener
2012-11-28 13:58 ` Richard Sandiford
2012-11-28 23:19 ` Eric Botcazou
2012-11-29 10:31 ` Richard Sandiford
2012-11-29 15:31 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).