Shouldn't we also have verifier checks for these requirements in the statically known cases?

In D103326#2824216, @foad wrote:

+more GlobalISel reviewers.

What do you all think about this? It seems to be the first generic combine designed specifically to run post-regbankselect. Should we be going in this direction? Or modifying existing combines so they can work both pre- and post-regbankselect?

Assuming we do want this combine, is there a neater way of writing the "ugly manual regbank preservation" that Matt highlighted?

In D104355#2822379, @qcolombet wrote:

Hi Amara,

I like this approach!
What is the impact on compile time?

Compile time seems to be noise, since this the new combine only runs in some cases where the unmerge combine fails, which is rare in most code. I would have liked to use it as the first-attempt combine for unmerge, but AMDGPU just wound up getting into a legalization loop (I think that's a problem with AMDGPU rather than the combine itself).

I am a little bit surprised by the long sequences that we generate now in certain case. What is the reason for that? (See inline comment for a highlight of what I am talking about.)

Cheers,
-Quentin

Remove commented out code.

Ok, LGTM.

In D101819#2798940, @sushmaunnibhavi wrote:

In D101819#2780892, @qcolombet wrote:

Hi,

LLVM ERROR: unable to translate in big endian mode (in function: f)

You'll need to turn that into a warning, otherwise effectively, none of your GISel changes will be testable (as in running the generated code on an actual target) until the support is complete, which is not a realistic constraint to have when bringing up a new GISel backend.

Cheers,
-Quentin

Hi @qcolombet ,
When I do a git grep with that error message, I find that it comes from IRTranslator.cpp file and in the runOnMachineFunction() function where it checks for big endian and throws a reportTranslationError() which is where that error comes from. By removing the reportTranslationError() temporarily in order to test my implementation, that would cause errors in other targets. Can you please suggest me any other way to solve this issue?

Thanks,
Sushma

You can add a hook to TargetLoweringInfo so that only targets which want to use big-endian in GlobalISel can enable it (just M68k for now). Then check this hook, if it false then do the current fallback code.

LGTM with minor nits addressed, thanks.

In D102869#2787196, @RKSimon wrote:

Are we already testing the G_MEMCPY path with large (invalid) sizes?

Anyone else got any comments?

In D102690#2767865, @peterwaller-arm wrote:

In D102690#2766984, @aemerson wrote:

Until the bug is fixed, this test cannot be re-enabled as a flaky test is worse than no test at all.

Thanks for chiming in. What I see is that the test is not currently disabled, but instead it falls back to testing for a default reference output and then fails:

https://github.com/llvm/llvm-test-suite/blob/3af2314126514c028cc39cd56510cd8badaba4e9/Makefile.programs#L871-L877

Is that consistent with what you see?

This doesn’t seem to happen on Darwin, so it’s a surprise that it’s just being noticed now. Either way makes sense to disable it in a better way.

I don't propose to re-enable the test until it is fixed -- at the moment the test is actively failing for myself and colleagues when running 'lnt runtest nt --test-suite llvm-test-suite'. Is there some other way to inhibit the test?

If so, I propose to do that.

I also worry for a moment in time that there is a window of time where *all* of these generated 'basic correctness tests' are disabled. It seems to me those tests could usefully catch bugs. Can we do better by only inhibiting the failing ones?

Sure. I did report this to Arm on multiple occasions, and as maintainers of the ACLE implementation I think Arm’s proprietary tool chain team should be taking responsibility for this to fix the issue.

In D102690#2766399, @peterwaller-arm wrote:

In D102690#2766200, @tmatheson wrote:

It was disabled here: https://github.com/llvm/llvm-test-suite/commit/87d67af9d8565d068b6706c081b7ae07addcb882

Is the underlying issue definitely fixed?

Thanks for raising that. I do not have any knowledge about the underlying issue at this time.

However, removing the output reference does not appear to inhibit the test, at least in my environment, it only causes lnt to use the default output which reads 'exit 0', hence the cryptic test failure message.

@aemerson are you able to comment, does the issue persist?

If it does persist, is there another way to inhibit or fix this exec test?

Guard this with a TLI check. I think a separate pass to do this is overkill.

In D101234#2758550, @aemerson wrote:

This seems to have broken the following code for AArch64, could you take a look?

target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
target triple = "arm64e-apple-ios14.0.0"

declare <3 x float> @bar(float)

define void @foo(float %a, float %b) {
entry:
  %call = call <3 x float> @bar(float undef)
  ret void
}

This seems to have broken the following code for AArch64, could you take a look?

@paquette See any issues with this?

Fair enough.

In D101538#2750853, @zuban32 wrote:

In D101538#2745606, @aemerson wrote:

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Well, our usecase might not be that canonical, let me briefly explain. For example, translating a bitcast - default IRTranslator implementation turns it into a COPY when LLTs of src and dst are the same. In our target (SPIR-V) LLTs do not matter that much, the typeinfo is represented with some pseudo instructions, so we run into this situation quite often. And losing a bitcast in the translator doesn't work for us as it'd obviously be quite difficult to recover it.

LGTM.

Wow, not running this at all for -Oz? LGTM.

Is this really necessary? Our preference is to use custom combines/lowering passes, or custom legalization, to do this.

Address comments. Factor out tryEmitBZero into the utils file.

Remove CSE from the combiner, this saves an additional 10% of the runtime in the pass, with no effect on code size.

Hadn't rerun test since an update. Move check to before the LI check so that it runs unconditionally.

Actually this seems to be already disabled on release builds, but my sampling was catching the empty set checks in analyzeDebugLocations(). Not a big enough deal to worry about.

In D101005#2725074, @foad wrote:

Looks fine to me. Could also do it for funnel shifts if you want.

Wow, that was a nasty one. Thanks for tracking it down. LGTM.

Remove constant folding check.

Only run this for AArch64.

In D101005#2711425, @foad wrote:

What's the point of this? Does it somehow end up generating better code?

It allows us to select immediate forms of rotate instructions for AArch64.

I would expect most targets to handle that during instruction selection by ignoring the high bits of the rotate amount, but perhaps there's some reason why it's awkward to do that for AArch64. Would it make sense for your new combine to be completely AArch64-specific? Currently you have added it to all_combines so it will be run on other targets too.

• Add more tests.
• Factor out the APInt checking part of the constant folder to be used as a query.
• Enable CSEInfo in the postlegalizer combiner so that we don't have to instantiate a constant-folding MIRBuilder.

In D101005#2707670, @foad wrote:

What's the point of this? Does it somehow end up generating better code?

If it is useful, you could also do the same for FSHL and FSHR.