LGTM.

In D128332#3612679, @aemerson wrote:

I'm curious, did we not already have support for these are part of the SVE2 ACLE implementation?

I'm curious, did we not already have support for these are part of the SVE2 ACLE implementation?

In D127853#3585792, @david-arm wrote:

In D127853#3585748, @aemerson wrote:

Can't the rdsvl immediate be used to do the scaling instead of a shift?

I think the rdsvl immediate specifies the multiplier, which equates to a shift left. Whereas I want to divide the length by the element size, i.e. a shift right.

Can't the rdsvl immediate be used to do the scaling instead of a shift?

No further comments from me, LGTM if Cullen approves.

+ @aprantl

Could you upload with more context (-U9999).

We can revisit the opaque pointers issue later. LGTM otherwise with a nit.

LGTM.

Hi @craig.topper, this looks like it broke under EXPENSIVE_CHECKS? https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/22475/

Can the test be reduced? Do you really need a main()?

Only fold undef shifts, not LHS.

In D125041#3500128, @foad wrote:

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

At the IR level, InstSimplify does this for all shifts (in the absence of exact/nsw/nuw flags). See SimplifyRightShift and SimplifyShlInst.

In D125041#3496339, @foad wrote:

In D125041#3495365, @aemerson wrote:

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

I don't understand why that would be true for SHL but not for the other shifts. What are the rules for undef in MIR? Is it like undef in IR, or like poison in IR, or neither?

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

In D121085#3371597, @yuntaopan wrote:

In D121085#3366519, @foad wrote:

In D121085#3365851, @aemerson wrote:

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

Shouldn't the select -> min/max/abs be done in IR, so there would be no need for this patch? See D98152 for example.

Our development branch does not have that patch yet, I have tried that patch and it works well. it's more appropriate to handle it at the IR and no need for this patch. But I have a question，why gisel can not rely on ValueTracking infrastructure ? Because gisel is based on mir, but ValueTracking is an IR analysis ？@aemerson

There's nothing wrong with using the ValueTracking infrastructure, the problem is that the only place we can use IR analysis are during the IRTranslator phase, and we try to avoid doing optimizations there as much as possible. Sometimes that might not be practical, but where we can do like to perform optimizations on generic MIR in combine passes instead.

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

LGTM.

I have no strong preference of whether we emit a copy or not FWIW.

LGTM.

LGTM with the test nit unless anyone else has objections.

Thanks for the feedback folks. To be honest I don't have the time right now to discuss and redesign the whole thing with David (have some parental leave coming up as well). If anyone else wants to pick this up and continue it feel free to do so, I published the patches to help other people with their debugging problems, but unless someone else picks this up and works to reach a consensus on design, it will have to lie unmaintained as a patch for Q1/Q2 next year at least.

LGTM, thanks.

LGTM. Can you rewrite the patch description for the commit message to more clearly explain what the actual problem this is solving too?

In D112852#3146936, @MatzeB wrote:

Requesting changes for the missing MIR serialization of the new property.

The issue here is determining where the barrier for disallowing these undefined vregs should be. LiveDebugVariables naturally discards these particular uses, so this is sufficient based on my knowledge.

Well to me this feels like we are making the intermediate representation more complicated (I consider it more complicated because DBG_VALUE instructions are special with lifetime rules now) for a very small gain.

That said I don't feel strongly about this. If others think this is benefitial then we can go ahead.

We haven’t changed the meaning of the MIR, we just clarified the existing semantics on the RFC thread. There’s no middle ground here as far as I can see, either it’s valid to have undefined uses or not. If it is, then we’re free to do anything that leaves them around.

In D113030#3109509, @dblaikie wrote:

In D113030#3103971, @aemerson wrote:

In D113030#3103903, @lebedev.ri wrote:

I would like to note that there's some rudimentary support
for previous generation of this scattered throught the codebase,
namely DebugCounters for bugpoint.

I don't really have an opinion on the proposal at large,
but i think it may be important to not just introduce a yet another variant
of dealing with the same issue, but only have a single good modern way.

Yes, I've seen DebugCounter. It's useful when you're already identified the file where something is going wrong, and you can enable the counters and pass counter values to opt. It doesn't however work across multiple TUs or support parallel debugging, so it's not solving the same problem.

Perhaps it'd be worth considering the overlap here, and maybe avoiding it: What would it be like if this new thing /only/ bisected at the granularity of a whole compilation action (ie: one invocation of clang), rather than specific optimizations? (possibly even only at the "whole compiler" granularity - using two compilers - and choosing between one or the other for each compilation)

Then once the specific file has been identified, use the existing sub-file granularity tools (a wrapper script could help cover both of these for the user so it wasn't a bunch more manual work).

I think that might help prevent overlap of functionality/re-implementing similar/the same functionality through different mechanisms for reducing specific optimization applications?

I think that restricting it to only allow bisection across translation units would be artificially constraining the functionality to not step on the toes of other features. That in itself doesn't seem the right approach, because the simplest thing right now is to support arbitrary bisection granularities. If we constrained it to only allow bisection down to TUs, then we haven't really simplified or re-used any code, all we've done is to make the user experience worse.

LGTM.

In D113030#3103903, @lebedev.ri wrote:

I would like to note that there's some rudimentary support
for previous generation of this scattered throught the codebase,
namely DebugCounters for bugpoint.

I don't really have an opinion on the proposal at large,
but i think it may be important to not just introduce a yet another variant
of dealing with the same issue, but only have a single good modern way.

In D113030#3103808, @qcolombet wrote:

Let me just step back a little bit and say that now that I think about what we did, having something that answers "should I run in this instance" is desirable, the implementation doesn't really matter. We did it with function attributes, but having a bisect client API like you're introducing is fine.
My only complain is that the client interface should not have remote in the name :P.

From an abstraction level, we need two things:

1. Something that tells if an optimization needs to run (the remote bisect client here)
2. Something that drives the on/off of the optimizations based on the previous state (here your daemon)

The way we did that was:
For #1 we added annotations in the IR
For #2 we implemented the driver directly in our JIT daemon

Essentially, that boils down to something that formulates a plan and something that executes the plan. At one point I was thinking that formulating the plan could be changing the pass pipeline (don't insert what you don't want to run), but that look like too much work :).

How does that work when you have parallel builds?

Each module is assigned an ID and the bisect plan and previous state are mapped to this ID.
The ID is saved in the module metadata but for now we didn't use it since all we needed was added to the IR via annotations (i.e., we didn't need to come up with a key to ask information about specific pass). In that regard, you're approach is more general.
For the ID, we used a hash of the module before the bisect annotations were added, i.e., as long as you don't change the front-end the ID are stable between runs.

To summarize:
Compute the module ID -> add annotation based on past information -> run the backend (at this point, the backend runs by itself.)

When multiple clang processes are running simultaneously, and you want to bisect to a specific translation unit, and then within that TU to a specific point in the module, don't you need some co-ordination?

At a high level here is what the driver was doing:

• Bisect optnone on each module
• Find the module(s) that creates the problem (the minimal set of modules that needs optimizations turn on)
• Do the same on each function (the minimal set may involve more than one function)
• Try to "outline" the basic blocks of each problematic function and do the same process on the newly created functions
• Split the problematic basic block to make them smaller and continue
• When you're happy with the size of the basic blocks, start bisecting the optimizations on the problematic functions (possibly basic block extracted). Right now we were only bisecting a handful of optimization because the final diff with the basic block splitting usually made the faulty optimization easy to find by hand.

The way it worked is all that state was saved in a file <shaderID>-bisect-info. You could bootstrap the process by populating the file by hand, i.e., by telling the JIT process which module you want to bisect.

As far as bisecting to a specific point in the TU, we were always going all the way down to the executable then you had to supply a script that tells whether or not the program is working. That's similar to what git bisect is doing (if the script runs 0, the program works, if that's one it doesn't). In your script you could check for whatever (specific sequence of asm, executable producing some results, etc.)

Note: The pass I was talking about in my previous reply that we insert in the LLVM pipeline, is generating all the information to start the bisect process (e.g., the shader ID, the list of all the functions, the list of all basic blocks). Then the driver was using this information to tell that pass to add some annotation on some function (e.g., optnone, noinline, etc.), but also to split some basic block and outline them (and attach some annotation on them).

Cheers,
-Quentin

In D113030#3103632, @qcolombet wrote:

Hi Amara,

I am kind of repeating what I said in https://reviews.llvm.org/D113031, but putting it here for better visibility.

I think the thing that drives the bisection shouldn't trickle down in the optimizations themselves, instead I would rather this information to be encoded in the IR itself (like a generalization of optnone).

For instance, for us a daemon based approach doesn't work at all because we are running a JIT compiler that runs in its own sandbox and we cannot query an external process from it.

Internally, we developed a bisect tool that annotates the IR upfront (to make an analogy with clang, this is as if clang would add a bunch of "bisect" attribute on the IR) before sending it for compilation instead of having the backend query a bisection client.
Note: technically we didn't modify clang, we instead had a specific pass at the beginning of the LLVM pipeline that would make all the bisection decisions, but also perform some transformation to isolate the bugs (like creating functions out of basic blocks, splitting the blocks to make the functions smaller, and blocking inlining.)

Cheers,
-Quentin

D113031 contains an example of a client for this in GlobalISel.

In D111132#3072726, @foad wrote:

In D111132#3071399, @arsenm wrote:

In D111132#3042221, @foad wrote:

2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

You mean, splitting a vector into vectors vs splitting a vector into scalars? Then why do we have both G_BUILD_VECTOR and G_CONCAT_VECTORS? The asymmetry annoys me!

In D111777#3071406, @arsenm wrote:

In D111777#3066274, @foad wrote:

No particular objection from me, though it would be more consistent if ConstantFoldUnaryOp built the G_CONSTANT, just like ConstantFoldVectorUnaryOp builds the G_BUILD_VECTOR.

That then raises the question of how you would call these functions from CombinerHelper match* functions, which don't want to build the IR until you get to the corresponding apply* function.

That then also raises the question of whether we really want to be doing constant folding both in the builder and in combiners.

I think the answer is yes, but for different reasons. I think constant folding in the builder only makes sense for handful of artifact-like operations, but general constant folding is always going to be needed in the combiners as other operations fold to constants

Please see the discussion here: https://groups.google.com/g/llvm-dev/c/R9lSoAqh5e4

Hoist end iterator eval in loop. Rebase on test check regeneration.

In D111524#3054451, @foad wrote:

No objections from me, but I do wonder if there's a way to apply this more consistently to all the constant folds, not just the binops.

Oops sorry about that, I didn't realize this was already on my branch when I committed 72ce310bf0de so this went in by mistake.