LGTM.

LGTM.

Maybe try to modify the existing G_XOR to be a G_AND instead of creating a new instruction?

I've decided to use custom legalization for this particular case in D88191, abandoning this change but I might revive it in another form later.

On further investigation I'm not sure this is the best way forward.

Add a test for ANDSWrs?

Nice clean up, LGTM.

In D87297#2264152, @qcolombet wrote:

Hi Amara,

dominates(A, B) tries to determine dominance by walking an iterator from the beginning of the block potentially until the end. In extremely large BBs this can result in very long compile times, since this is called often from the artifact combiner.

Instead of disabling the check, I think it would be best to fix the underlying compile time issue. The solution here would be to switch the list of MachineInstr to numbered instructions so that dominance checks become O(1).
I had meant to look at this for quite some time but didn't get a chance to do it.
Something similar has been done on LLVM IR (see https://reviews.llvm.org/D51664). I wish it was done in a way that could have benefited the Machine IR like I suggested in https://reviews.llvm.org/D51664#1389884, but this ship has sailed!

I understand this is a big under taking so I am fine with the threshold approach in the meantime.

I leave the final approval to Aditya and Matt.

Cheers,
-Quentin

Ping

I remember you mention that the change to CallLowering approach to this issue resulted in worse perf or code size. That does seem odd to me since we should be just aping SDAG, so even if we had some regressions we shouldn't be worse overall.

Ping

Ping

In D86871#2254760, @jonpa wrote:

Patch updated per review.

I'm questioning why we should have that "isDefined" logic in the first place. It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.

I changed te uses in the AMDGPU backend so that it is now obvious that the only reason for having isDefined() is to allow SelectionDAGBuilder to set the flags at a later point than when creating the nodes.

To get rid of it I think this is needed:

• Refactor SelectionDAGBuilder to call setValue(DAG.getNode()) with the correct SDNodeFlags directly.
• SelectionDAGBuilder::Visit(const Instruction) call to setFlags() can be removed.
• the isDefined() and AnyDefined member of SDNodeFlags can then be removed.

I still believe that even with that change most (all?) uses of setFlags are dangerous as they could introduce a flag into a shared node that may not be appropriate for all existing uses. But maybe that can then be a separate discussion elsewhere.

I may be missing something, but as long as the memoization does a true intersection of the flags per this patch, there should not necessarily be anything broken, or?

I think it would be possible to make things clearer:

• When a new equivalent node takes over flags from a node it is replacing (for instance during widening), it should pass the flags to getNode() directly or if there are many calls to getNode() maybe call a function like transferFlags(OrigNode, NewNode) at the end of the function.
• When a new node is created and the values of the flags are given by the context, the flags should be passed directly to getNode().
• When a single flag needs to be set on an existing node, why not do it directly instead of calling getFlags(), set...(), setFlags() (like SelectionDAGISel currently does for setNoFPExcept).
• ...?

I added a TODO comment referencing this.

I agree removing the isDefined() check within intersectWith() is a good idea, and if we can do it without regression, we should do so. (And that will certainly fix the original problem Jonas was seeing.

Yes, all the tests are passing :-) With fast-fp, there are 2 files changing on SPEC with a total of just 8 less fused ops (without fast fp, there is no change). This is due to SelectionDAGBuilder now clearing the flags of the fmul and fadd nodes so DAGCombiner will not produce the fma. I would hope this is temporarily acceptable, or?

@aemerson: It seems you introduced the isDefined() with d28f0cd4 - are you OK with this change, and what are your comments now?

I don't have any strong feelings about this. I agree with Sanjay that correctness is more important but it would be good to have some tests that show what we're now failing to optimize.

LGTM.

In D86383#2250882, @rupprecht wrote:

Thanks!

Usually, tests that are marked # REQUIRES: asserts are done so because they make use of some code that is only available behind NDEBUG. However, in this case, it's outright crashing on certain inputs when not in debug mode. At a first glance, that doesn't seem like a correct fix to me -- after all, the crashing bug is still there -- but I'm not familiar with this code, so I'm probably missing something?

Thanks, it seems the test doesn't work without asserts enabled. I've re-committed it fixed in 520ab710fb6f9829b4e70fda1dcc91ed4f614d0a

Did you commit this?

Ping

Ping,

Ping

Rebase and move getICmpTrueVal() from an earlier patch to this one.

Simplify and refactor code with FP case.

Actually I'm going to abandon this. I think later CodeGen passes can eliminate these and make better decisions about it.

In D86786#2244826, @arsenm wrote:

In D86786#2244814, @aemerson wrote:

computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.

It is correct to match zextload though, you just know 1 fewer bit vs. sextload. It doesn't matter what the source is. This is exactly what DAGCombiner does: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11054

computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.

Address comments.

Address comments.

Add support for vectors.

In D86664#2240598, @arsenm wrote:

The IRTranslator chooses to omit the G_BR if it can use a fallthrough. Should it not do that either, and the verifier enforce always having G_BR and G_BRCOND?

Why isn't this caught in InstCombine?

In D86665#2240582, @arsenm wrote:

I was actually just looking at the original globalisel proposal, and switches were intended to be directly translated into a G_SWITCH instruction. Would it be much effort to move this into a machine transformation?

For AMDGPU we're stuck using LowerSwitch and control flow intrinsics, but in the near-ish future we can directly handle control flow here