In D146031#4206072, @foad wrote:

I wonder if you need to update getAliasResult in AMDGPUAliasAnalysis for the new address space.

It seems arc didn't want to update the revision Summary from my commit message, so I'll add this as a comment:
I'm also planning to add this paragraph to the commit message:

Moved the new address space to 128, and clarified that it is an internal address space that can be freely renumbered.

Thanks for catching those :)

Include new tests in check-all

In D144176#4131548, @foad wrote:

cross-project-tests is new to me, but this certainly looks pretty useful - thanks!

In D142279#4088057, @foad wrote:

I've investigated a bit more. It's a difference between libstdc++-11 and libstdc++-12, caused by this patch:

https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=a54ce8865a885bca5ab9c4aa6ec725cd13c09901

Previously a failed assertion would print a message with __builtin_printf. Now it prints a message by calling __glibcxx_assert_fail which is defined in the library.

What's the way forward for the flang runtime library? Can we disable libstdc++ assertions? Can we provide our own implementation of __glibcxx_assert_fail?

Sorry, I just noticed this, I'll have a look.

Thanks for the updates, LGTM with nits.

Smaller functions FTW, regardless of whether or not we ever override them :)

Why do we need a new pass? Could we achieve the same goals with a new RegBankSelect mode instead? (Sorry if this was discussed somewhere else and I missed it, in any case I think the commit message should either explain the rationale or point to wherever it's been discussed)

Add tests with undef

In D141902#4058303, @foad wrote:

This should make it easier to match complex immediates for certain
instructions on AMDGPU, where for instance a <8 x s16> will be split
before instruction selection into a G_CONCAT_VECTORS of <2 x s16>
splats.

I don't quite understand when this would be useful. If <8 x s16> is split up into <2 x s16> pieces during legalization, because only the latter is legal, then why would we still have a G_CONCAT_VECTORS with result type <8 x s16>? What operations would use that result? Do you have an example in mind?

This looks very cool! Are there any plans to run it during the pre-merge checks?

In D138323#3949485, @Rot127 wrote:

In D138323#3948239, @0x59616e wrote:

Wouldn't this break the 'open-close principle' and 'Interface segregation principle' ?

Indeed it does. How about removing the PrinterInterface and make the PrinterLLVM the base class for all extensions (and mark its methods as virtual). Other Printers inherit from PrinterLLVM and override the methods needed?
This wouldn't be a big change, but also keep the printing logic in one place and not spread around the backends.

In D141372#4042701, @foad wrote:

In D141372#4040553, @scott.linder wrote:

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

In D141372#4040553, @scott.linder wrote:

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

Thanks for the quick review! I'm going to leave this around until tomorrow, in case anyone wants to bikeshed about the new name.

Rebase.

In D140964#4026229, @arsenm wrote:

We should have a separate buildMergeValues and buildMerge could use a less confusing name

In D140964#4025539, @foad wrote:

This patch simplifies the API of MachineIRBuilder so that the
buildInstr method does the least surprising thing (i.e. builds an instruction
with the specified opcode) and only the convenience buildX methods
(buildMerge etc) are allowed freedom over which opcode to use.

I like this!

Does this patch actually fix any missed CSE opportunities?

Nice tests! :) I have one microscopic nit and one question (because I don't know an awful lot about config files). Thanks for working on this.

In D135853#3871993, @vzakhari wrote:

Thanks! Looks good to me, though, if/when we have multiple such cases we will need to simplify the addition of function names and their mappings.

Address review comments.
Thanks for the feedback :)

Hi! This is a rework of https://reviews.llvm.org/D134860

Right, I see what you mean. I'll rework this, thanks!

I'm planning to add more tests (see TODO), but I thought it would be good to get some general feedback on the approach first. @Meinersbur had also suggested adding a hypotf to the runtime when compiling for Windows, but this approach seems more flexible and straightforward to me. Other opinions welcome, thanks!

In D133561#3808337, @ro wrote:

In D133561#3792286, @rovka wrote:

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

The new testcase PASSes on sparcv9-sun-solaris2.11 and sparc64-unknown-linux-gnu. However, for 32-bit sparc it runs into an assertion failure

Assertion failed: llvm::all_of(types, [](Type t) { return t; }) && "attempting to construct a TypeRange with null types", file /vol/llvm/src/llvm-project/local/mlir/lib/IR/TypeRange.cpp, line 20

and I don't yet see what's wrong. Any suggestions or do I need to run a full Debug build to investigate?

IMO this is a reasonable approach, I only have a few nits.

Hi, I have a couple of questions:

• Are you actually testing on all these platforms? I don't think I've seen any BSD binaries for LLVM 15.
• Do you plan to add any buildbots with flang on *BSD?
• How different is the ABI from plain Linux? Will you need any changes in the handling of Complex / Character types (see the target-rewrite-* tests like this one) ?

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

In D130734#3777469, @cabreraam wrote:

I do not yet have commit access! How do I go about requesting that?

LGTM! Thanks for putting up with so many rounds of review. I have 2 more nits, but you don't have to submit another diff. Do you have commit access?

Thanks for getting back to this, it's starting to look very nice and slender! :) I have a few more comments, all pretty minor.

LGTM.

@gkistanova Can we please have a buildmaster restart to include this? This should fix the failures we're seeing on https://lab.llvm.org/buildbot/#/builders/197
Thanks & sorry about the trouble!