Fix the nits ( -> /, Optional<std::pair<...>>, etc.)

LGTM with a nit.

Made all requested changes in the commit rL305791

Constrain the source operand too. This fixes the odd GPR64all, it's now GPR64sp.

Resubmit the patch minus the bits that were already in D33590. Sorry about that.

Update the SelectionDAG rules for FPR->GPR bitcasts to use GPR32all and GPR64all

The remaining issue I mentioned in the update is this one.

• Double check why tablegen gives us gpr32 instead of gpr32all

Fix all the nits.

Fix the X86 machine verifier bug where EXTRACT_SUBREG could emit a subregister
copy with classes that didn't quite support it. TableGen now finds a suitable
pair of classes. However, if this results in a copy of the operand into another
class, tablegen will currently refuse to import SelectionDAG because we lack
multi-insn emission at the moment. Support for that will start to be introduced
as part of the state-machine patches.

This was reverted due to compile-time regressions so re-open it and add it to the end of the patch series that fixes that.

In D33758#769689, @kristof.beyls wrote:

Replace the matcher if-statements for each rule with a state-machine. This
significantly reduces compile time, memory allocations, and cumulative memory
allocation when compiling AArch64InstructionSelector.cpp.o after r303259 is
recommitted.

My guess is that the compiler compile time goes down simply because tablegen produces far less code?

After re-testing this on current trunk, there's a bug in X86 due to GR64 not fully supporting the 8bit subregister index. I'll fix this shortly using failedImport() for this case since we don't have multi-insn emission quite yet. The following patches introduce the infrastructure for it.

Factored out the optimization into MachineIRBuilder.

LGTM

LGTM.

Is it possible to add a test case for this? It seems that a test where the last instruction has a DILocation should trigger it.

One nit: We could avoid introducing the Optional<> by testing for LLT.isValid() instead of testing for whether the Optional has a value

In D32869#752657, @qcolombet wrote:

Hi Daniel,

We are talking pass each other :).

LGTM

I haven't looked too closely at the output of the macro since I don't know what it should be but it looks like the issues I raised earlier in the review are no longer there. I just have one nit about nextReg().

In D32869#746617, @qcolombet wrote:

If that happens, doesn't that means we need to support it?

The way I was thinking about this was that we should be able to insert copy to vreg before and after to assign to and from the physical registers.

I believe PrintTo will be valid since it's occurs after the instruction we're working on. If we're folding multiple instructions into one we'll be following the operands backwards, away from PrintTo.

Hoist out the checks for intrinsic id's so that they are only importable when
found in the place we expect them to be.

Refresh patch

In D32744#745035, @aditya_nandakumar wrote:

Just to clarify, this wouldn't work if the target decided to insert new instructions before PrevMII (or even at the beginning for e.g.). I'm guessing if the target does that, its not a big deal if we don't print it.

Thanks for doing this. It will allow me to add a test for the predicates related to the optsize attribute.

In D32491#740829, @rovka wrote:

I just checked and the existing ARM tests fail without this patch because the available features aren't reset when entering the functions. They work with this patch + enabling the TableGen selector for ARM, so we'll have tests then :)

In that case I'll commit the previous version of this patch tomorrow and post the immediate support for a separate review. The PR can then come out of that review. Thanks

Add support for patterns consisting solely of an IntInit* and use this to add a
test case to the rule predicate support... almost.

Moved ForCodeSize and similar to the Subtarget.

I'm having trouble improving the test case since the function-level predicates don't show up in any importable rules yet. I'll see if I can uncover one

LGTM

In D32491#737781, @rovka wrote:

So, we have a new GISelAccessor with a new InstructionSelector for each unique subtarget (module + function attributes, as hashed in getSubtargetImpl). Why do we need to have module features and function features in each InstructionSelector? Isn't there a different InstructionSelector for each kind of function, and don't we magically get the right one when processing each function? What am I missing? It seems to me that we could get away with a single set of availableFeatures per InstructionSelector.