LGTM.

LGTM.

In D24722#603980, @sdardis wrote:

@vkalintiris this patch ok without the dependency and FIXMEs? I should be able to get the other patch in as I can provide cleaner test cases.

LGTM. I have one question/suggestion inline.

Thanks for reviewing this! I committed this in r287646.

LGTM.

LGTM.

LGTM.

LGTM but there's a wrong constant that you should change before committing this.

I didn't consider all the cases but it seems that the macro expansion is probably fine in terms of correctness. However, the expansion is different from that of GAS' and this will generate different object files unnecessarily. As far as I can tell, GAS handles these macro in 3 different ways depending on the value of the immediate:

LGTM.

In D25438#576494, @sdardis wrote:

From immediates-bad.ll:

define void @addvi_d(<2 x i64> * %ptr) {
entry:
; CHECK-LABEL: addvi_d:
; CHECK: addv.d
  %a = load <2 x i64>, <2 x i64> * %ptr, align 16
  %r = call <2 x i64> @llvm.mips.addvi.d(<2 x i64> %a, i32 65)
  store <2 x i64> %r, <2 x i64> * %ptr, align 16
  ret void
}

will crash LLVM. Dump from llc:

Assertion failed: (width > BitWidth && "Invalid APInt ZeroExtend request"), function zext, file /Users/simon/dev/llvm/llvm2/llvm/lib/Support/APInt.cpp, line 981.
Stack dump:
0.	Program arguments: /Users/simon/dev/llvm/llvm2/llvmgitsvnbuild/./bin/llc -march=mips -mattr=+msa,+fp64 -relocation-model=pic 
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@addvi_d'

As you can seeI think there may be another solution though, which is to constrain the type of the immediate operand down to things like i5. I would have to reinvestigate.

We generate different code from the GNU assembler which in some cases is wrong, see inline comment.

LGTM once its dependency lands.

Why shouldn't we "crash" when we pass invalid/bad constant immediates to our intrinsics? Do you have a case in mind that causes us problems if we do so?

LGTM.

LGTM.

LGTM.

LGTM.

Marking this with "Request Changes" based on my last comment.

When I tested this I noticed that MultiSource/Benchmarks/7zip/7zip-benchmark was failing. I'm marking this with "Request Changes" to better keep track of the pending review requests.

LGTM. See inline comments for a few small changes.

Cleanup FastISel support checks and remember to reject it on MIPS16 as well.

I'm getting a few test failures from the LLVM test-suite for MIPS32R2. Can you take a look at what's going wrong and update the patch before I review it? Given the symmetry of the code I'd expect, more or less, the same test failures to happen for microMIPS32R2 too but I'm not sure.

In D24510#553802, @seanbruno wrote:

In D24510#553700, @vkalintiris wrote:

Hi Sean, is your intention to LGTM this patch by accepting it, or to convey that it's working fine for *BSDs?

I had hoped to express that it is an improvement to our FreeBSD toolchain. I cannot comment reliably on quality outside of style or the ability to be able to apply this review to a trunk checkout.

LGTM.

Hi Sean, is your intention to LGTM this patch by accepting it, or to convey that it's working fine for *BSDs?

We should split this in two separate review requests, ie. one that introduces symbol dumping and another that marks local and weak symbols. Can you also add a separate test file just for checking the newly introduced -asm-show-symbol-table option?

Abandoning this because arc didn't add llvm-commits.

LGTM.

In D24674#547589, @vkalintiris wrote:

@sdardis: can you update the review request before committing this because I don't think that test/CodeGen/builtins-mips-msa.c has ever been properly reviewed before.

@sdardis: can you update the review request before committing this because I don't think that test/CodeGen/builtins-mips-msa.c has ever been properly reviewed before.

I removed myself from the reviewers because I'm not quite familiar with this area. @joerg I added you as reviewer because you've already commented, feel free to undo my change if you don't have time to review this.

LGTM with a few minor changes, see inline comments.

Can you take a look at CodeGen/Mips/cconv/arguments-varargs.ll because it fails for me (and rebase the patch while at it :))?

LGTM with one comment/question inline.

LGTM but make sure that the checks from the new test enforce the correct ordering of loads/stores.

When building with BUILD_SHARED_LIBRARIES, such tests don't manage
to find their libraries and are consequently not run (due to a bug
in LIT, this isn't even reported!).

Hi Sebastian. I tried this on one of our BB that tests recursive builds and test/Transforms/GVN/pr28626.ll is failing. I'm getting the same failure on an X86_64 host.

Thanks! LGTM.

The code changes LGTM but the test case doesn't look like it is testing the right thing.

LGTM with two small test fixes.

LGTM.

From what I remember it boils down to the fact that we don't want to have atomics whose width isn't natively supported on our targets. Take a look at the comments from this thread: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/328321.html.

LGTM with the addition of one test for the M3 prefix.

Hi Nitesh, you can go ahead and commit this. But you should change the comments from:

LGTM.

LGTM. I have just one question inline.