LGTM

LGTM. Thanks.

LGTM. Thanks.

It looks like this resolves PR/53722 not 53772.

LGTM

Do you have commit access?

In D120305#3337045, @atanasyan wrote:

In D120305#3337019, @MaskRay wrote:

@atanasyan A few mips test/Driver tests will fail. Wonder if you have time making them more portable... Otherwise I'll just add UNSUPPORTED: default-pie-on-linux to them.

Failed Tests (6):
  Clang :: Driver/hip-fpie-option.hip
  Clang :: Driver/mips-cs.cpp
  Clang :: Driver/mips-fsf.cpp
  Clang :: Driver/mips-img-v2.cpp
  Clang :: Driver/mips-img.cpp
  Clang :: Driver/mips-mti-linux.c

I will take a look at the tests. Probably hip-fpie-option.hip is unrelated to MIPS?

In D120305#3337019, @MaskRay wrote:

@atanasyan A few mips test/Driver tests will fail. Wonder if you have time making them more portable... Otherwise I'll just add UNSUPPORTED: default-pie-on-linux to them.

Failed Tests (6):
  Clang :: Driver/hip-fpie-option.hip
  Clang :: Driver/mips-cs.cpp
  Clang :: Driver/mips-fsf.cpp
  Clang :: Driver/mips-img-v2.cpp
  Clang :: Driver/mips-img.cpp
  Clang :: Driver/mips-mti-linux.c

LGTM with a small fix. I will commit the changes shortly.

LGTM. Thanks for the patch.

Thanks for the patch. Some notes are below.

Thanks for review.

• Reduce redundancy of the test case
• Add IsMips64 field to the Object class.
• Save reference to an Object in the RelocationSection class.

Since the "Mips64EL" property is a property of the input object, I wonder if we should just make it a member of the object,
and then put a reference to that object in the relocation sections, so that we can query that, rather than needing
to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

LGTM with some nits.

LGTM

The patch committed at rG302a165

Do you have commit access?

• Does this problem exist on all versions of Debian or starting from specific version only?
• This fix needs test cases. Take a look at "Check linker invocation on Debian 6 MIPS 32/64-bit" in the clang/test/Driver/linux-ld.c for example.

LGTM. But get one more acceptance from any other reviewer before commit.

LGTM. Thanks for the fix.

Thanks for the patch.

LGTM. Do you have commit access?

• This patch needs test cases. Take a look at llvm/unittests/ADT/TripleTest.cpp
• IMHO the current fix is not the best solution. It solves the problem but looks a bit strange. For example, the following code (unusual but correct) stops working. Without the patch it produces a correct "arm-linux-gnu" target triple.

Triple T("mipsisa32r6-linux-gnu");
T.setArch(Triple::arm);

It's better to modify Triple::setArch() function so it takes two arguments ArchType and SubArchType. The second argument is NoSubARch by default. Then the setArch() passes both arguments to the getArchTypeName(). We need to teach this function to generate architecture's name in accordance with ArchType and SubArchType arguments. In this patch we can do it for MIPS only. @MaskRay - what do you think?

If we construct T = Triple("mipsisa64r6el-unknown-linux-gnuabi64") what is result of the T.getArchName() call (without your patch)? If getArchName() returns mipsisa64r6el we need to investigate what is the reason for redundant setArch() call somewhere in the clang driver code. If getArchName() does not return mipsisa64r6el we need to fix triple parsing. One more question - why other targets which has "sub architectures" does not need to extend the Triple::setArch method?

LGTM

In D109359#2999558, @c-rhodes wrote:

Patches fixing warnings have either landed or are in review for all but AMDGPU and MIPS now. @tstellar please could you take a look at the warnings in the AMDGPU backend or tag another contributor? Understand you might be busy with the 13 release.

For compatibility with tools unsupported 64-bit pc-relative relocations the patch introduces new command line options in Clang: mmips-pc64-rel and mno-mips-pc64-rel. These options passed to LLVM by Clang driver as -mmips-pc64-rel={true|false}. I could not find any better way to pass the option into MC layer.

Thanks for review.

• Check section name prefix .debug_ instead of section type. This approach is closer to other MIPS toolchains.

In D107707#2933312, @MaskRay wrote:

MIPS .debug_* sections should have SHT_MIPS_DWARF section type to distinguish among sections contain DWARF and ECOFF debug formats

If SHT_PROGBITS is a valid value, now what's the value keeping using SHT_MIPS_DWARF?

Thanks for review.

LGTM

LGTM

LGTM

The MIPS changes LGTM

LGTM

LGTM

LGTM

LGTM

AFAIK the MIPS LLVM-based toolchain mentioned in D13340 have not been implemented.

In D98659#2631768, @jrtc27 wrote:

I don't know why it only affects microMIPS (presumably it does its lowering subtly differently from normal MIPS) but those changes look sensible to me

LGTM

LGTM

I see. Thanks. One more question - is it possible to split the patch: one for "fixing a couple of rounding issues" and another for "Introducing a few more conversion rules"?

I have not checked the patch carefully yet. But it looks like some tests was removed. For example MIPS64R5EL-LABEL: i8_2... Is it intended?

In D80392#2537794, @joewholden wrote:

Hi guys,

any chance of this making it into 11.1.0 or 12?

@ntesic Do you have commit access?

LGTM

LGTM

My bad, I missed that you implemented passing the option to backend.

What's a goal of this change? Do you want to suppress an error message when the option provided to Clang? If so, is it a real-life case?

LGTM with a few nits. Thanks.

MIPS changes LGTM

LGTM

LGTM. Thanks!

LGTM

LGTM

LGTM

LGTM

LGTM