[Bug target/103383] New: Microblaze bswaphi2

[Bug target/103383] New: Microblaze bswaphi2

[rkujoth at motorolasolutions dot com]

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

            Bug ID: 103383
           Summary: Microblaze bswaphi2
           Product: gcc
           Version: 6.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rkujoth at motorolasolutions dot com
  Target Milestone: ---

Came across a weird issue in which GCC tried to stick a bswaphi2 operation in
the delay slot of a beqid. Since the bswaphi2 operation is actually two
Microblaze instructions, it only executed the first instruction when taking the
branch (the byte swap -- swapb).

I don't know anything about target configurations, but I found the target macro
here:
https://github.com/gcc-mirror/gcc/blob/master/gcc/config/microblaze/microblaze.md?plain=1#L369-L375

Looks like support for swaphi2 was added in this commit:
https://github.com/gcc-mirror/gcc/commit/cb8a1637a9914fb25bfbc9604fc20ca5e6c0601e

But, there was an error...it mapped swaphi2 to the Microblaze swaph
instruction, which only performs a half-word swap. That was fixed in this
commit:
https://github.com/gcc-mirror/gcc/commit/e50f5f2e0d3396bd4ab2f799135309de39c96d58

But, I wonder if the "TARGET_REORDER" should have been removed, since the
implementation requires two Microblaze instructions?

Not really sure, but adding -fno-delayed-branch to my compile resolves the
issue for me.

I'm using mb-gcc from Vivado 2017.1, which is based on gcc 6.2.0.

[Bug target/103383] Microblaze bswaphi2

[rkujoth at motorolasolutions dot com]

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

--- Comment #1 from Rich <rkujoth at motorolasolutions dot com> ---
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?

[Bug target/103383] Microblaze bswaphi2

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
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

[kujoth at gmail dot com]

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

--- Comment #3 from Rich Kujoth <kujoth at gmail dot com> ---
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]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(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 can cause issues with delay slots

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Microblaze bswaphi2         |Microblaze bswaphi2 can
                   |                            |cause issues with delay
                   |                            |slots

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
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 can cause issues with delay slots

[kujoth at gmail dot com]

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

--- Comment #6 from Rich Kujoth <kujoth at gmail dot com> ---
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

[eager at eagercon dot com]

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

--- Comment #7 from Michael Eager <eager at eagercon dot com> ---
Do you have a test case which shows the problem?

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

[rkujoth at motorolasolutions dot com]

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

--- Comment #8 from Rich <rkujoth at motorolasolutions dot com> ---
(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 <stdint.h>
#include <stdlib.h>

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

[rkujoth at motorolasolutions dot com]

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

--- Comment #9 from Rich <rkujoth at motorolasolutions dot com> ---
of course, that should be -mxl-barrel-shift...