LGTM :-)

LGTM with a note about clang-format

LGTM

The full patch. The previous update was taken from the wrong machine and hadn't
been finished. Aside from not compiling, there was also a bug where additional
combine attempts would see G_SEXTLOAD mutate into G_LOAD (the anyext form).

Rewrite the patch such that the extends hoist up to the loads. The previous
patch had a couple problems that became clear on the bots:

• The loads could be duplicated. This is a correctness problem for volatile loads but more generally is likely to harm performance.
• The loads would sink to the extends without any hazard checking

Re-opening as I'm about to update it with a revised version. The previous one broke several bots because it sank loads down to the extends

Generally LGTM. I have a couple questions on this one though

LGTM

LGTM

Having the side-effect inside the assert is harmless since the #ifndef ensures the whole loop is only built when asserts are enabled and Index is only used by the assert. However, I agree it's weird to have side-effects inside the assert. Moving the increment out of the assert and into the for loop would LGTM

Yes, I do have my doubts that the original implementation with sign extension is always correct, but it's NFC here anyway. I'm thinking to take a look at it at some point and see if I can come up with a breaking test case for that sign extend.

Update before commit

I can see some optimizations for G_SEXTLOAD/G_ZEXTLOAD since they're only legal when they're an extend so they could just drop the check in the optimized table.

I think we lost 0.5% on compile time on CTMark because of this. Is there any scope left here for performance improvements? With the addition of the combiner landing later, I expect a further CT hit so anything we can do here would be good.

Thanks

I have some performance figures from the LLVM test-suite now using the --benchmarking-only option. Here's the summaries reported by lnt:

I've only gone through the ABI side of things (and from memory) but I have a couple comments.

LGTM

In D45990#1084328, @tstellar wrote:

In D45990#1084322, @dsanders wrote:

In D45990#1084299, @tstellar wrote:

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

Could you send me the comment that's shortly before the GIR_CopyFConstantAsFPImm in AMDGPUGenGlobalIsel.inc? It should look similar to a tablegen pattern and we can probably derive a test case from that. I'm not sure which rule has been imported but I'm hoping it will be something like (set $rd:GPR, $imm:fpimm) in which case the test would be similar to test/CodeGen/AArch64/GlobalISel/select-constant.mir.

Here is the comment. Note that the reason I don't have a test case is because this pattern is only for the R600 sub-target and I'm not implementing global-isel for that sub-target:

// MIs[0] Operand 1
// No operand predicates
// (fpimm:{ *:[f32] }):$val  =>  (MOV_IMM_F32:{ *:[f32] } (fpimm:{ *:[f32] }):$val)
GIR_BuildMI, /*InsnID*/0, /*Opcode*/AMDGPU::MOV_IMM_F32,
GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
GIR_CopyFConstantAsFPImm, /*NewInsnID*/0, /*OldInsnID*/0, // va

Thanks. I'd suggest making it a little more obvious to someone skim-reading the list, for example:

[GlobalISel][InstructionSelect] Refactoring out a getMatchTable virtual method + other small NFC's

but LGTM either way.

LGTM

The NFC changes look good to me but the commit message (particularly the subject line) doesn't really match the content. LGTM with a better commit message.

In D45990#1084299, @tstellar wrote:

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

LGTM. Preferably with a test case in this patch but adding the test cases along with the AMDGPU patches you're working on will also be ok

It's worth mentioning that:

%1:_(s64) = G_INTTOPTR %0(<4 x s16>)

isn't a valid G_INTTOPTR. The result should be a pointer type and the input should be a scalar. However, your point stands even with that fixed.

Refresh patch
Fix most of the nits
Update GlobalISelEmitter.td

In D45543#1066099, @aditya_nandakumar wrote:

Maybe we could have all the tests in one file called prelegalize-combine-extloads.mir

Thanks. I'll commit this when I'm back in the office on Thursday

Thanks. I'll commit this (with the nits fixed) when I'm back in the office on Thursday

Aside from being a large patch with few function-level comments, some of the non-functional changes also make this difficult to review. It would be helpful to move things like the indentation correction on testImm*(), the introduction of buildTable and getMatchTable(), the changes to coverage, moving the emission of selectImpl() down, etc. into a separate patch(es).

Sorry for the delay, I was out of the office for a few days.

Rebase to trunk

I'm not sure why it's still separate but having it separate means that compilers with only targets that don't implement GlobalISel don't spend time initializing the passes for it. I think Tom's guess about minimizing LLVM_BUILD_GLOBAL_ISEL is probably the original reason.

I think you're ok to commit

LGTM

Ah, I'm reading the patches out of order :-). 1.5s seems reasonable

Also, could you double check the memory usage for this. Some of the bots have a fairly limited amount of memory so the 'ninja check' time will be amplified on those if it consumes a lot.

Those are quite large tests. How much time does it add to 'ninja check'? If it's reasonably short (I don't really have a number in mind for this) then LGTM. Otherwise we may need to think about trimming it down a bit

Those are quite large tests. How much time does it add to 'ninja check'? If it's reasonably short (I don't really have a number in mind for this) then LGTM. Otherwise we may need to think about trimming it down a bit

The GenericOpcodes.td change makes sense to me but I'm surprised that only AMGGPU's LegalizerInfo is changing and only for SHL. I'd have expected most (probably all) GlobalISel targets to need to change all three shifts.

Sorry for taking a while to get to this. It's been a busy couple weeks

Remove unnecessary std::string constructor

Statistics can actually be gathered (LLVM_ENABLE_STATS isn't always false anymore). Tested out of the patch using #error in the unit test.
Moved StatisticInfo back

Fix a race between GetStatistics() and addStatistic()
Documented caveats that are the callers responsibility to handle if it matters to them.

In D43901#1024928, @rtereshin wrote:

LGTM

My only concern here is that I don't quite understand why only the ManagedStatic<StatisticInfo> StatInfo's writer is guarded by a mutex, but non of the readers. As the newly added GetStatistics() method leaks a read-access to StatInfo into the wild losing any control over it, it might be a problem.

Could you please clarify that a little bit?

Remove unintentional change. ResetStatistics() is something I'm considering implementing later