| Message ID | n994mj585o1.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
On 09/08/2015 06:17 AM, Jiong Wang wrote: > > All other cost helper functions are using signed int to hold cost > while seq_cost is using unsigned int. > > This fix this. bootstrap OK on x86. > > OK for trunk? > > 2015-09-08 Jiong Wang <jiong.wang@arm.com> > > gcc/ > * rtl.h (seq_cost): Change return type from "unsigned" to "int". > * rtlanal.c (seq_cost): Likewise. Why not go the other way and start making things unsigned -- for a cost function like this, unsigned seems more natural to me. jeff
Jeff Law writes: > On 09/08/2015 06:17 AM, Jiong Wang wrote: >> >> All other cost helper functions are using signed int to hold cost >> while seq_cost is using unsigned int. >> >> This fix this. bootstrap OK on x86. >> >> OK for trunk? >> >> 2015-09-08 Jiong Wang <jiong.wang@arm.com> >> >> gcc/ >> * rtl.h (seq_cost): Change return type from "unsigned" to "int". >> * rtlanal.c (seq_cost): Likewise. > Why not go the other way and start making things unsigned -- for a cost > function like this, unsigned seems more natural to me. I was using "(unsigned) -1" to represent maximum cost, then later known there is MAX_COST macro and found it's actually signed type, and quick search shows most of the code in gcc/rtlanal.c, are using "int", so I am changing seq_cost which seems to be the only cost helper using unsigned. And looks like there is no consistent type to hold cost value across gcc, some are using "unsigned" while others are using "int", I guess they are surviving because gcc disable -Wconversion by default.
On 09/08/2015 09:17 AM, Jiong Wang wrote: > > Jeff Law writes: > >> On 09/08/2015 06:17 AM, Jiong Wang wrote: >>> >>> All other cost helper functions are using signed int to hold cost >>> while seq_cost is using unsigned int. >>> >>> This fix this. bootstrap OK on x86. >>> >>> OK for trunk? >>> >>> 2015-09-08 Jiong Wang <jiong.wang@arm.com> >>> >>> gcc/ >>> * rtl.h (seq_cost): Change return type from "unsigned" to "int". >>> * rtlanal.c (seq_cost): Likewise. >> Why not go the other way and start making things unsigned -- for a cost >> function like this, unsigned seems more natural to me. > > I was using "(unsigned) -1" to represent maximum cost, then later known > there is MAX_COST macro and found it's actually signed type, and quick > search shows most of the code in gcc/rtlanal.c, are using "int", so I am > changing seq_cost which seems to be the only cost helper using unsigned. Understood. But the natural type should be unsigned as far as I can tell. The fact that we're using signed types all over the place is probably a historical wart. So I'd start by changing the MAX_COST macro to an unsigned type, then fix any fallout from that. That should be a patch unto itself. Then we can have additional follow-up patches to fix the types of costing related variables, parameters & return values. Jeff
diff --git a/gcc/rtl.h b/gcc/rtl.h index ac56133..ded054c 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -3050,7 +3050,7 @@ extern rtx_insn *find_first_parameter_load (rtx_insn *, rtx_insn *); extern bool keep_with_call_p (const rtx_insn *); extern bool label_is_jump_target_p (const_rtx, const rtx_insn *); extern int insn_rtx_cost (rtx, bool); -extern unsigned seq_cost (const rtx_insn *, bool); +extern int seq_cost (const rtx_insn *, bool); /* Given an insn and condition, return a canonical description of the test being made. */ diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c index ef98f4b..2f14b93 100644 --- a/gcc/rtlanal.c +++ b/gcc/rtlanal.c @@ -5160,10 +5160,10 @@ insn_rtx_cost (rtx pat, bool speed) /* Returns estimate on cost of computing SEQ. */ -unsigned +int seq_cost (const rtx_insn *seq, bool speed) { - unsigned cost = 0; + int cost = 0; rtx set; for (; seq; seq = NEXT_INSN (seq))