On 03/17/2017 04:14 PM, Segher Boessenkool wrote:
Thanks for not cc:ing me on any of this.
There's really no need for getting upset. Bernd posted the message to
the patches list. That's sufficient.
On Wed, Mar 15, 2017 at 04:00:21PM +0100, Bernd Schmidt wrote:
+/* Set up a set of registers used in an insn. Called through note_uses,
+ arguments as described for that function. */
+
+static void
+record_used_regs (rtx *xptr, void *data)
+{
+ bitmap set = (bitmap)data;
Space after cast, throughout.
A nit. Bernd, can you please fix this.
+ int i, j;
+ enum rtx_code code;
+ const char *fmt;
+ rtx x = *xptr;
+
+ /* repeat is used to turn tail-recursion into iteration since GCC
+ can't do it when there's no return value. */
+ repeat:
This comment is incorrect.
Depends on the situation. GCC's ability to turn tail recursion into
iteration is not good.
+ /* If we are about to do the last recursive call
+ needed at this level, change it into iteration.
+ This function is called enough to be worth it. */
This function is called enough that it should not be called at all.
I disagree. We have a code gen bug. We need a solution. Feel free to
come up with something better.
+ /* For combinations that may result in two insns, we have to gather
+ some extra information about registers used, so that we can
+ update all relevant LOG_LINKS later. */
Please just refuse to do the combination in such cases instead.
?!? That would essentially mean we can't do 3->2 combinations.
+ /* See comments above where we calculate the bitmap. */
+ EXECUTE_IF_SET_IN_BITMAP ((bitmap)new_regs_in_i2,
+ LAST_VIRTUAL_REGISTER, i, iter)
Why do you need a cast here at all?
+ {
+ rtx reg = regno_reg_rtx[i];
+ rtx_insn *other;
+ for (other = NEXT_INSN (i2); other != i3; other = NEXT_INSN (other))
+ if (NONDEBUG_INSN_P (other)
+ && (reg_overlap_mentioned_p (reg, PATTERN (other))
+ || (CALL_P (other) && find_reg_fusage (other, USE, reg))))
+ {
+ if (dump_file)
+ fprintf (dump_file,
+ "found extra use of reg %d at insn %d\n", i,
+ INSN_UID (other));
+ insn_link **plink;
+ for (plink = &LOG_LINKS (other);
+ *plink;
+ plink = &(*plink)->next)
+ {
+ insn_link *link = *plink;
+ if (link->regno == i)
+ {
+ *plink = link->next;
+ link->next = i3links;
+ i3links = link;
+ break;
+ }
+ }
+ break;
+ }
+ }
This should be a separate function.
Bernd, can you pull this out into its own function.
So, no, I'm not okay with this. It is very expensive, it is doing open
heart surgery on combine's internal structures (in a way that may or may
not work), and all that to combine some insns in a case that should not
exist anyway.
Please don't be dramatic. If you've got a better suggestion for how to
fix this, be my guest. I don't like the compile-time cost either, but I
don't see a better solution.
Jeff
