RE: [PATCH v2 1/1] opcodes: microblaze: Add new bit-field instructions

["Frager, Neal" <neal.frager@amd.com>]

> Hi Michael, Maciej,
> 
>> Neal --
> 
>> Please resubmit this patch.
> 
>> Add test cases for new instructions.
>> Fix GNU Coding Standard issues.
>> Run binutils test suite.
> 
> Thank you for feedback.  Could you possibly help me with resolving the GNU coding standard issues and the build issue Maciej is reporting?

> See other messages for GNU Coding Standards.

> Maciej did not report a build issue: there are test case regressions.
> See his instructions on running the binutils test suite.

> 
> Currently, the problem we are facing is that the upstream binutils is unable to build the zynqmp pmufw application.  I am trying to resolve this by upstreaming the toolchain patches AMD is currently using in its microblaze toolchain release.
> 
> This is the only patch remaining that is preventing the zynqmp pmufw application from building, as the application needs the microblaze bsefi and bsifi instructions.
> 
> My patch for adding these instructions is coming from the following two patches which AMD currently applies on its own microblaze toolchain that comes with our yocto releases:
> https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/reci
> pes-devtools/binutils/binutils/0004-LOCAL-Fix-relaxation-of-assembler-
> resolved-reference.patch 
> https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/reci
> pes-devtools/binutils/binutils/0008-Add-new-bit-field-instructions.pat
> ch

> I don't have time to research patches in a out-of-tree repository.  I did look at the other patches you submitted to see if there was some information about what problem the patch was solving and found nothing.
> If there is relevant information in these other patches, include it with your patch.

Hi Michael,

You should not need to look at these patches.  Everything implemented by these patches has been included in the patch I submitted.  I only mentioned them to show where the patch I submitted originated from.

> 
> As I am far away from being a toolchain or GNU binutils expert, could you please help me in the simplest of terms understand what is not following the GNU coding standard in these patches, so that I can fix it?

> See other message.

I just submitted v4 of the patch with the following updates:
  - Cleaned up all of the GNU coding standard issues.
  - Removed any line modifications which were unnecessary, so v4 of the patch looks much cleaner now.
  - Tested this patch on the AMD functional side in that it not only builds, but the built binutils are able to build the zynqmp pmufw application correctly.
  - Verified that the 32-bit host machine build issue is resolved.

As far as I can tell, the only thing missing is a test case for the gas testsuite to verify the relocation works as expected.

Would you be ok with applying the current v4 of the patch while I work on this last part?

> 
> My current way of testing is verifying that the master branch build of GNU binutils is able to build the zynqmp pmufw application and that it executes correctly on zynqmp hardware.  So I have verified on hardware that the v2 version of the patch I submitted is indeed working.  If you could help me with resolving these regressions, so that this patch can solve the problem I am trying to solve without causing another problem, I would appreciate any support you can provide.

> That may be satisfactory for AMD's requirements, but your patch has to 
> work with other targets.  I'm happy to help you, but my time is limited. 
>  You (or someone else) will have to do the heavy lifting to resolve 
> regressions.

> For each of the test suite failures which Maciej reported, reproduce the 
> problem outside of the test suite.  I believe there are error messages.

Are you sure that the problems Maciej reported can be reproduced outside of the test suite?  If so, how?

> 
> Also, are there any instructions for coding standard for generating test cases for the bsefi and bsifi instructions?  As I have never written GNU binutils test cases before, perhaps you can point me to something that will help me to save time with this as well?

> To test the functions in your patch, you will need to create a small 
> assembler test case which invokes the correct relocations.  The bare 
> bones "is the opcode correct" test which you added will not do this.

> I'll take a look at the MB test cases and see if there is one you can 
> adapt.

Understood and thank you for your help.  I will work on this now.
 
Thank you for your support!
 
Best regards,
Neal Frager
AMD