>
> Patch bootstrapped and regression tested on arm-none-linux-gnueabihf,
> however, on my native Aarch32 setup the test times out when run as part
> of a big "make check-gcc" regression, but not when run individually.
>
> 2019-10-11 Stamatis Markianos-Wright <stam.markianos-wri...@arm.com>
>
> * config/arm/arm.md: Update b<cond> for Thumb2 range checks.
> * config/arm/arm.c: New function arm_gen_far_branch.
> * config/arm/arm-protos.h: New function arm_gen_far_branch
> prototype.
>
> gcc/testsuite/ChangeLog:
>
> 2019-10-11 Stamatis Markianos-Wright <stam.markianos-wri...@arm.com>
>
> * testsuite/gcc.target/arm/pr91816.c: New test.
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index f995974f9bb..1dce333d1c3 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -570,4 +570,7 @@ void arm_parse_option_features (sbitmap, const
> cpu_arch_option *,
>
> void arm_initialize_isa (sbitmap, const enum isa_feature *);
>
> +const char * arm_gen_far_branch (rtx *, int,const char * , const char *);
> +
> +
Lets get the nits out of the way.
Unnecessary extra new line, need a space between int and const above.
> #endif /* ! GCC_ARM_PROTOS_H */
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 39e1a1ef9a2..1a693d2ddca 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -32139,6 +32139,31 @@ arm_run_selftests (void)
> }
> } /* Namespace selftest. */
>
> +
> +/* Generate code to enable conditional branches in functions over 1 MiB. */
> +const char *
> +arm_gen_far_branch (rtx * operands, int pos_label, const char * dest,
> + const char * branch_format)
Not sure if this is some munging from the attachment but check
vertical alignment of parameters.
> +{
> + rtx_code_label * tmp_label = gen_label_rtx ();
> + char label_buf[256];
> + char buffer[128];
> + ASM_GENERATE_INTERNAL_LABEL (label_buf, dest , \
> + CODE_LABEL_NUMBER (tmp_label));
> + const char *label_ptr = arm_strip_name_encoding (label_buf);
> + rtx dest_label = operands[pos_label];
> + operands[pos_label] = tmp_label;
> +
> + snprintf (buffer, sizeof (buffer), "%s%s", branch_format , label_ptr);
> + output_asm_insn (buffer, operands);
> +
> + snprintf (buffer, sizeof (buffer), "b\t%%l0%d\n%s:", pos_label, label_ptr);
> + operands[pos_label] = dest_label;
> + output_asm_insn (buffer, operands);
> + return "";
> +}
> +
> +
Unnecessary extra newline.
> #undef TARGET_RUN_TARGET_SELFTESTS
> #define TARGET_RUN_TARGET_SELFTESTS selftest::arm_run_selftests
> #endif /* CHECKING_P */
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index f861c72ccfc..634fd0a59da 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -6686,9 +6686,16 @@
> ;; And for backward branches we have
> ;; (neg_range - neg_base_offs + pc_offs) = (neg_range - (-2 or -4) + 4).
> ;;
> +;; In 16-bit Thumb these ranges are:
> ;; For a 'b' pos_range = 2046, neg_range = -2048 giving (-2040->2048).
> ;; For a 'b<cond>' pos_range = 254, neg_range = -256 giving (-250 ->256).
>
> +;; In 32-bit Thumb these ranges are:
> +;; For a 'b' +/- 16MB is not checked for.
> +;; For a 'b<cond>' pos_range = 1048574, neg_range = -1048576 giving
> +;; (-1048568 -> 1048576).
> +
> +
Unnecessary extra newline.
> (define_expand "cbranchsi4"
> [(set (pc) (if_then_else
> (match_operator 0 "expandable_comparison_operator"
> @@ -6947,22 +6954,42 @@
> (pc)))]
> "TARGET_32BIT"
> "*
> - if (arm_ccfsm_state == 1 || arm_ccfsm_state == 2)
> - {
> - arm_ccfsm_state += 2;
> - return \"\";
> - }
> - return \"b%d1\\t%l0\";
> + if (arm_ccfsm_state == 1 || arm_ccfsm_state == 2)
> + {
> + arm_ccfsm_state += 2;
> + return \"\";
> + }
> + switch (get_attr_length (insn))
> + {
> + // Thumb2 16-bit b{cond}
> + case 2:
> +
> + // Thumb2 32-bit b{cond}
> + case 4: return \"b%d1\\t%l0\";break;
> +
> + // Thumb2 b{cond} out of range. Use unconditional branch.
> + case 8: return arm_gen_far_branch \
> + (operands, 0, \"Lbcond\", \"b%D1\t\");
> + break;
> +
> + // A32 b{cond}
> + default: return \"b%d1\\t%l0\";
> + }
Please fix indentation here.
> "
> [(set_attr "conds" "use")
> (set_attr "type" "branch")
> (set (attr "length")
> - (if_then_else
> - (and (match_test "TARGET_THUMB2")
> - (and (ge (minus (match_dup 0) (pc)) (const_int -250))
> - (le (minus (match_dup 0) (pc)) (const_int 256))))
> - (const_int 2)
> - (const_int 4)))]
> + (if_then_else (match_test "TARGET_THUMB2")
> + (if_then_else (and (ge (minus (match_dup 0) (pc)) (const_int -250))
> + (le (minus (match_dup 0) (pc)) (const_int 256)))
> + (const_int 2)
> + (if_then_else (and (ge (minus (match_dup 0) (pc))
> + (const_int -1048568))
> + (le (minus (match_dup 0) (pc)) (const_int 1048576)))
> + (const_int 4)
> + (const_int 8)))
> + (const_int 10)))
> + ]
This conditional is unreadable and is getting quite complex.
Please fix the indentation and add some comments to indicate when
this is 2, 4, 8, 10 above the pattern and ask for the comment to
be in sync with this.
How did we end up with length 10 ? That indicates 2 4 byte instructions
and a 2 byte instruction ? You are handling lengths 2, 4, 8 above in
the switch - is length 10 going to be a single A32 b<cond> instruction ?
What am I missing ?
> )
>
> (define_insn "*arm_cond_branch_reversed"
> @@ -6978,17 +7005,36 @@
> arm_ccfsm_state += 2;
> return \"\";
> }
> - return \"b%D1\\t%l0\";
> + switch (get_attr_length (insn))
> + {
> + // Thumb2 16-bit b{cond}
> + case 2:
> +
> + // Thumb2 32-bit b{cond}
> + case 4: return \"b%D1\\t%l0\";break;
> +
> + // Thumb2 b{cond} out of range. Use unconditional branch.
> + case 8: return arm_gen_far_branch \
> + (operands, 0, \"Lbcond\", \"b%d1\t\");
> + break;
> + // A32 b{cond}
> + default: return \"b%D1\\t%l0\";
> + }
> "
> [(set_attr "conds" "use")
> (set_attr "type" "branch")
> (set (attr "length")
> - (if_then_else
> - (and (match_test "TARGET_THUMB2")
> - (and (ge (minus (match_dup 0) (pc)) (const_int -250))
> - (le (minus (match_dup 0) (pc)) (const_int 256))))
> - (const_int 2)
> - (const_int 4)))]
> + (if_then_else (match_test "TARGET_THUMB2")
> + (if_then_else (and (ge (minus (match_dup 0) (pc)) (const_int -250))
> + (le (minus (match_dup 0) (pc)) (const_int 256)))
> + (const_int 2)
> + (if_then_else (and (ge (minus (match_dup 0) (pc))
> + (const_int -1048568))
> + (le (minus (match_dup 0) (pc)) (const_int 1048576)))
> + (const_int 4)
> + (const_int 8)))
> + (const_int 10)))
> + ]
Same comments as above apply here too.
Ramana
> )
>
>
> diff --git a/gcc/testsuite/gcc.target/arm/pr91816.c
> b/gcc/testsuite/gcc.target/arm/pr91816.c
> new file mode 100644
> index 00000000000..176bf61780b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr91816.c
> @@ -0,0 +1,102 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=armv7-a -mthumb -mfpu=vfpv3-d16" } */
> +int printf(const char *, ...);
> +
> +__attribute__((noinline,noclone)) void f1(int a)
> +{
> + if (a) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW0
> + }
> +}
> +
> +__attribute__((noinline,noclone)) void f2(int a)
> +{
> + if (a) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW3
> + }
> +}
> +
> +
> +__attribute__((noinline,noclone)) void f3(int a)
> +{
> + if (a) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW5
> + }
> +}
> +
> +__attribute__((noinline,noclone)) void f4(int a)
> +{
> + if (a==1) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW0
> + }
> +}
> +
> +__attribute__((noinline,noclone)) void f5(int a)
> +{
> + if (a==1) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW3
> + }
> +}
> +
> +
> +__attribute__((noinline,noclone)) void f6(int a)
> +{
> + if (a==1) {
> +#define HW0 printf("Hello World!\n");
> +#define HW1 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
> +#define HW2 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
> +#define HW3 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
> +#define HW4 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
> +#define HW5 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> + HW5
> + }
> +}
> +
> +
> +int main(void)
> +{
> + f1(0);
> + f2(0);
> + f3(0);
> + f4(0);
> + f5(0);
> + f6(0);
> + return 0;
> +}
> +
> +
> +/* { dg-final { scan-assembler-times "beq\\t.L\[0-9\]" 2 } } */
> +/* { dg-final { scan-assembler-times "beq\\t.Lbcond\[0-9\]" 1 } } */
> +/* { dg-final { scan-assembler-times "bne\\t.L\[0-9\]" 2 } } */
> +/* { dg-final { scan-assembler-times "bne\\t.Lbcond\[0-9\]" 1 } } */
> +/* { dg-final { scan-assembler-times "b\\t.L\[0-9\]" 2 } } */
