[Bug target/103383] Microblaze bswaphi2 can cause issues with delay slots

[rkujoth at motorolasolutions dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #9 from Rich  ---
of course, that should be -mxl-barrel-shift...

[Bug target/103383] Microblaze bswaphi2 can cause issues with delay slots

[rkujoth at motorolasolutions dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #8 from Rich  ---
(In reply to Michael Eager from comment #7)
> Do you have a test case which shows the problem?

It's not difficult to get it to show up if you're using the builtin swap16 and
conditionals. Here's a simple one (the optimizations make writing a simple test
case weird):

main.c:

#include 
#include 

int main()
{
  uint16_t src1 = (uint16_t)rand();
  uint16_t src2 = (uint16_t)rand();
  if(__builtin_bswap16(src1) != rand())
  {
return -1;
  }
  return 0;
}

Then using the Vivado 2017.1 toolchain...
mb-gcc -O2 -mcpu=v10.0 -mlittle-endian -xl-barrel-shirt main.c

Then I see the following sequence in a.out, which shouldn't happen:
brlid
swapb
swaph

[Bug target/103383] Microblaze bswaphi2 can cause issues with delay slots

[eager at eagercon dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #7 from Michael Eager  ---
Do you have a test case which shows the problem?

[Bug target/103383] Microblaze bswaphi2 can cause issues with delay slots

[kujoth at gmail dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #6 from Rich Kujoth  ---
I'm not really in a position to change toolchains, so I needed to make it work
with the version of GCC I have. Since the issue is specific to swap16, I made
my own swap16 function that explicitly calls the two microblaze instructions,
rather than using the GCC built-in. This resolves the issue and I don't see any
instances of swapb/swaph following a delayed branch in the compiled code.

[Bug target/103383] Microblaze bswaphi2 can cause issues with delay slots

[pinskia at gcc dot gnu.org via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

Andrew Pinski  changed:

   What|Removed |Added

Summary|Microblaze bswaphi2 |Microblaze bswaphi2 can
   ||cause issues with delay
   ||slots

--- Comment #5 from Andrew Pinski  ---
Note I have no way to test the patch which I put here. I hope someone who
understands Microblaze better and has a reasonable way to test it to take over
this bug report. Also the maintainer of Microblaze seems not to be very active
either ...

[Bug target/103383] Microblaze bswaphi2

[pinskia at gcc dot gnu.org via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #4 from Andrew Pinski  ---
(In reply to Rich Kujoth from comment #3)
> Haha nice, I guess I should have read a little further. Thanks!
> 
> What does the "length" mean?

length means the size of the instructions in bytes that would be in the
executable. I noticed it was not set so it might be a good idea to set it too.
There are places in the Microblaze backend that uses the length attr to add
some nops or something I didn't really look into it that much.

[Bug target/103383] Microblaze bswaphi2

[kujoth at gmail dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #3 from Rich Kujoth  ---
Haha nice, I guess I should have read a little further. Thanks!

What does the "length" mean?

[Bug target/103383] Microblaze bswaphi2

[pinskia at gcc dot gnu.org via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

Andrew Pinski  changed:

   What|Removed |Added

 Target||microblaze
   Last reconfirmed||2021-11-24
 Status|UNCONFIRMED |NEW
 Ever confirmed|0   |1

--- Comment #2 from Andrew Pinski  ---
I think this patch fixes the problem:
diff --git a/gcc/config/microblaze/microblaze.md
b/gcc/config/microblaze/microblaze.md
index 6d77752cbfd..ca4bd976a5c 100644
--- a/gcc/config/microblaze/microblaze.md
+++ b/gcc/config/microblaze/microblaze.md
@@ -372,7 +372,8 @@ (define_insn "bswaphi2"
   "TARGET_REORDER"
   "swapb %0, %1
swaph %0, %0"
-)
+  [(set_attr "length"  "8")
+   (set_attr "type" "no_delay_arith")])

 ;;
 ;; Microblaze delay slot description


 CUT 
The delay slot description is right below bswaphi2 definition even:
(define_delay (eq_attr "type" "branch,call,jump")
  [(and (eq_attr "type"
"!branch,call,jump,icmp,multi,no_delay_arith,no_delay_load,no_delay_store,no_delay_imul,no_delay_move,darith")
(ior (not (match_test "microblaze_no_unsafe_delay"))
 (eq_attr "type" "!fadd,frsub,fmul,fdiv,fcmp,store,load")
 ))
  (nil) (nil)])

Or multi could be used instead of no_delay_arith.

[Bug target/103383] Microblaze bswaphi2

[rkujoth at motorolasolutions dot com via Gcc-bugs]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103383

--- Comment #1 from Rich  ---
Seems like I probably misunderstood the meaning of "TARGET_REORDER" -- looks
like it's a Microblaze build option and this keyword appears to be there to
indicate that these instructions only exist with a certain set of build options
(which are the default).

So, perhaps the issue I'm seeing is a bit more generic? How does GCC perform
the delayed branch optimization? If an operation requires multiple
instructions, how does GCC know not to stick it in the branch delay slot?