In D141135#4046071, @aemerson wrote:

In D141135#4036756, @Kai wrote:

• Visit of changed instructions in the Combiner is now done iteratively.
• The array with the rules to execute in the generated source is a bit more compact.

Running LLVM test suite with -fglobal-isel on an AArch64 EC2 instance (2 CPUs, 4GB):
Current implementation: 22m43.224s
Bottom-up implementation: 22m47.165s

Size of the binaries:
Current implementation: llc 127.280.024, clang 217.540.416
Bottom-up implementation: llc 127.026.960, clang 217.291.456

I haven't had time to properly look at the implementation, but a question on this. When you say "running the test suite", are you talking about building the test suite with -fglobal-isel? Not running the generated code right?

• Visit of changed instructions in the Combiner is now done iteratively.
• The array with the rules to execute in the generated source is a bit more compact.

I restructured the YAML output used for the test. As a result I could remove the sorting code, which caused a longer running time of the algorithm. The construction of the matcher is much faster now. However, testing got more challenging. I also made some minor changes in other parts of the algorithm. The generated matcher code is unchanged..

In D141135#4031488, @arsenm wrote:

Adding the new parameter to tryCombineAll() should possibly be a separate PR. I think that introducing a new class CombinerState, which is just a thin wrapper around a DenseMap<MachineInstr *, unsigned>, would also make sense.

Like D81899?

In D141135#4031482, @arsenm wrote:

We can also just update tests. Avoiding test changes isn't a good reason to do something

In D141135#4031482, @arsenm wrote:

Compiling LLVM is slightly longer with this change applied: 25m35.471s vs 28m51.797

This is huge. I don't understand this part. Surely you don't have bootstrapped globalisel working? Why this slowdown?

@nemanjai Any comments?

In D140119#4000412, @shchenz wrote:

hmm, so many duplicated codes, but seems there is no simple way to avoid this.

D139813 adds some improvement for i64 materialization in DAG ISel. That should be adopted to GISel too.

This reminds me there was a proposal (https://reviews.llvm.org/D82709#2137837) for PPC target that PPC can materialize the i64 imm after ISEL and in ISEL just lower the i64 imm to a pseudo instruction. So we can avoid the duplication here and also some other remat issue in register allocation. But that may require big effort...

• Use uint32_t in findContiguousZerosAtLeast()
• Replace use of constrainSelectedInstRegOperands() with constrainAllUses()
• Uses std::optional<bool> instead of MI parameter

In D140107#3998002, @RKSimon wrote:

This works for the ppc64le/ppc64be triples?

Completely reworked the change.

In D139444#3988544, @probinson wrote:

Searching the buildbot console page for 'zos' turns up nothing; 's390' turns up clang-s390x-linux, clang-s390x-linux-lnt, mlir-s390x-linux. Are there any zos hosted bots? I.e., is this test ever actually run? Maybe there are only downstream zos bots, in which case this should be a downstream test instead?

In D139444#3978189, @uweigand wrote:

In D139444#3975182, @probinson wrote:

The changes in this patch assume that there aren't any possible suffixes after the -zos part of the triple (no version numbers, like you might find with darwin or macos, and nothing like -elf or -eabi like some targets have). If there are suffixes, I'll happily revise to put {{.*}} after everything.

I think for consistency with other targets, and to be safe for future extensions of the target triple, it would be better to add the {{.*}}

In D139444#3982205, @probinson wrote:

If you can tell me the platform.system() value to look for to detect z/OS, I can do that. Probably as a separate patch, as it would be going beyond the mechanical replacement that I'm doing for everything else.

@shchenz Sorry for the delay.
I copied the selectCopy() function from your change. I also added a simplified version of getRegClass(). That is hopefully good enough.

Added selectCopy() implementation from D132942

Fix formatting.

Some nit comments but in general looks good to me.

Moved implementation out of anonymous name space in PPCCallLowering.cpp.

Let the generic code handle TargetOpcode::COPY.

• update selectCopy() to constraint all registers
• regenerate test using -ppc-asm-full-reg-names option

Add comment (again? hm.).

Fix formatting complain from clang-format.

• Adds a resize when adding the leaves which do not care about the instruction to all partitions, as it is done already in addToPartition()
• Adds a comment to the test. The test just follows the edges in the graph, to make sure the construction is correct.

The change creates some regressions on the buildbots which I was not yet able to reproduce.

Update comment.

• Remove unused functions in CombineHelper
• Add test checking source code

I made another fix which enables matching of linear patterns.
To showcase the functionality I changed 3 simple combine rules
to use linear matching. All changed rules are covered by existing
tests.

Remove unused Inserted variable.

In D133257#3768704, @tschuett wrote:

Did you forget the test case?

Added missing test.

LGTM with the message in the assert changed.