This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Add known bits to InstructionSelector
ClosedPublic

Authored by arsenm on Aug 28 2019, 6:41 PM.

Details

Summary

AMDGPU uses this for some addressing mode selection patterns. The
analysis run itself doesn't do anything so it seems easier to just
always require this than adding a way to opt in.

Diff Detail

Event Timeline

arsenm created this revision.Aug 28 2019, 6:41 PM

Hi Matt, this looks reasonable - though when (if) we add a more complex implementation (perhaps precomputes knownbits) we might have to revisit this.
For now, it's close to zero cost. LGTM.

This revision is now accepted and ready to land.Aug 28 2019, 8:52 PM
arsenm closed this revision.Aug 29 2019, 10:23 AM

r370388

This change has caused some test failures in our Armv7 only builders http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-full-sh and http://lab.llvm.org:8011/builders/clang-cmake-armv7-full . An example build failure is http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/7366

All the errors are in global isel tests and have the form:

Pass 'InstructionSelect' is not initialized.
Verify if there is a pass dependency cycle.
Required Passes:
	Target Pass Configuration

It is reproducible on an X86_64 machine, although only when -DLLVM_TARGETS_TO_BUILD="ARM" and only ARM (the bot configuration). If I include AArch64 as well as ARM then the tests pass. I presume this change has added a pass dependency that is not being initialised in the ARM target, but it does if there is another Target that does initialize it. I note that the ARM equivalent of AArch64InstructionSelector doesn't call have the equivalent of setupMF.

Can you take a look?

I note that the ARM equivalent of AArch64InstructionSelector doesn't call have the equivalent of setupMF.

That isn't it. InstructionSelector isn't the pass itself (InstructionSelect is), and setupMF has a default implementation

I note that the ARM equivalent of AArch64InstructionSelector doesn't call have the equivalent of setupMF.

That isn't it. InstructionSelector isn't the pass itself (InstructionSelect is), and setupMF has a default implementation

Agreed. I arrived at this patch via bisection. I've done a few more experiments and it seems like it is not just the ARM Global Isel tests that depend on the AArch64 backend. If I build the X86 and AMDGPU targets without AArch64 then some of their Global Isel tests fail aswell. Last bit of naive speculation before I go home. The only place in the codebase with INITIALIZE_PASS_DEPENDENCY(GISelKnownBitsAnalysis) is in AArch64PreLegalizerCombiner.cpp. Does this need to be added to the other backends that use Global Isel?

I note that the ARM equivalent of AArch64InstructionSelector doesn't call have the equivalent of setupMF.

That isn't it. InstructionSelector isn't the pass itself (InstructionSelect is), and setupMF has a default implementation

Agreed. I arrived at this patch via bisection. I've done a few more experiments and it seems like it is not just the ARM Global Isel tests that depend on the AArch64 backend. If I build the X86 and AMDGPU targets without AArch64 then some of their Global Isel tests fail aswell. Last bit of naive speculation before I go home. The only place in the codebase with INITIALIZE_PASS_DEPENDENCY(GISelKnownBitsAnalysis) is in AArch64PreLegalizerCombiner.cpp. Does this need to be added to the other backends that use Global Isel?

This is missing from InstructionSelect

dyung added a subscriber: dyung.Aug 30 2019, 10:12 AM

Is someone working on a fix for this that will go in soon? I am about to revert this change because it is blocking our internal builds as well as breaking both of the public PS4 bots.

Is someone working on a fix for this that will go in soon? I am about to revert this change because it is blocking our internal builds as well as breaking both of the public PS4 bots.

I have a patch I'm waiting for tests to commit