Page MenuHomePhabricator

GlobalISel: Don't materialize immarg arguments to intrinsics
ClosedPublic

Authored by arsenm on Feb 14 2019, 5:20 AM.

Details

Summary

Encode them directly as an imm argument to G_INTRINSIC*.

Since now intrinsics can now define what parameters are required to be
immediates, avoid using registers for them. Intrinsics could
potentially want a constant that isn't a legal register type. Also,
since G_CONSTANT is subject to CSE and legalization, transforms could
otentially obscure the value (and create extra work for the
selector). The register bank of a G_CONSTANT is also meaningful, so
this could throw off future folding and legalization logic for AMDGPU.

This will be much more convenient to work with than needing to call
getConstantVRegVal and checking if it may have failed for every
constant intrinsic parameter. AMDGPU has quite a lot of intrinsics wth
immarg operands, many of which need inspection during lowering. Having
to find the value in a register is going to add a lot of boilerplate
and waste compile time.

SelectionDAG has always provided TargetConstant for constants which
should not be legalized or materialized in a register. The distinction
between Constant and TargetConstant was somewhat fuzzy, and there was
no automatic way to force usage of TargetConstant for certain
intrinsic parameters. They were both ultimately ConstantSDNode, and it
was inconsistently used. It was quite easy to mis-select an
instruction requiring an immediate. For SelectionDAG, start emitting
TargetConstant for these arguments, and using timm to match them.

Most of the work here is to cleanup target handling of constants. Some
targets process intrinsics through intermediate custom nodes, which
need to preserve TargetConstant usage to match the intrinsic
expectation. Pattern inputs now need to distinguish whether a constant
is merely compatible with an operand or whether it is mandatory.

The GlobalISelEmitter needs to treat timm as a special case of a leaf
node, simlar to MachineBasicBlock operands. This should also enable
handling of patterns for some G_* instructions with immediates, like
G_FENCE or G_EXTRACT.

I haven't finished updating all of the targets yet. I handled X86
first, figuring it would be the worst to handle. Most of the remaining
work is for hexagon, but SystemZ also needs a non-trivial change.

There is one crash remaining in GlobalISelEmitter when using timm for
a standalone Pat for an ARM intrinsic.

Diff Detail

Event Timeline

arsenm created this revision.Feb 14 2019, 5:20 AM
ab accepted this revision.Feb 15 2019, 2:00 PM

Nice!

lib/CodeGen/GlobalISel/IRTranslator.cpp
1178

Might be worth using enumerate() (STLExtras.h)

This revision is now accepted and ready to land.Feb 15 2019, 2:00 PM
ab requested changes to this revision.Feb 15 2019, 2:39 PM

On second thought, what happens to the tablegen emitter? That needs to be taught about these as well, right? That might not trigger on anything right now though.

This revision now requires changes to proceed.Feb 15 2019, 2:39 PM
In D58232#1399984, @ab wrote:

On second thought, what happens to the tablegen emitter? That needs to be taught about these as well, right? That might not trigger on anything right now though.

Similarly, there's functions like isOperandImmEqual() (which is what the generated ISel matcher uses) that are expecting immediates to be a G_CONSTANT.

arsenm updated this revision to Diff 210150.Jul 16 2019, 12:42 PM
arsenm retitled this revision from GlobalISel: Don't materialize immarg arguments to intrinsics to WIP: GlobalISel: Don't materialize immarg arguments to intrinsics.
arsenm edited the summary of this revision. (Show Details)

Work on fixing tablegen emitter and make matching DAG changes

arsenm updated this revision to Diff 210343.Jul 17 2019, 8:47 AM

Fix other target failures except hexagon, which uses generated td files.

Sort of worked around the ARM crash, which is the commented out case in immarg.td. The problem was using a "timm" input pattern, and an "imm:" in the result pattern. TableGen has never made it clear what things are valid inputs or outputs, and I don't think this is handled consistently. This does work out in the DAG emitter, so I don't know if this should be allowed or an error. It does error if you try to use timm in an output.

atanasyan added inline comments.Jul 17 2019, 11:00 AM
include/llvm/Target/TargetSelectionDAG.td
816 ↗(On Diff #210343)

s/wrapepr/wrapper/

arsenm updated this revision to Diff 215761.Aug 17 2019, 4:47 PM
arsenm retitled this revision from WIP: GlobalISel: Don't materialize immarg arguments to intrinsics to GlobalISel: Don't materialize immarg arguments to intrinsics.

Manually fix hexagon

arsenm updated this revision to Diff 215762.Aug 17 2019, 4:55 PM

Use common tablegen test target

ping. This is blocking most of my patches

paquette added inline comments.Aug 26 2019, 9:49 AM
test/TableGen/immarg.td
27–31 ↗(On Diff #215762)

Does this still crash?

Would it be possible to make it just error for now, saying it's currently unsupported?

arsenm marked an inline comment as done.Aug 26 2019, 4:25 PM
arsenm added inline comments.
test/TableGen/immarg.td
27–31 ↗(On Diff #215762)

Yes. It hits this:

InstructionMatcher &
RuleMatcher::getInstructionMatcher(StringRef SymbolicName) const {
  for (const auto &I : InsnVariableIDs)
    if (I.first->getSymbolicName() == SymbolicName)
      return *I.first;
  llvm_unreachable(
      ("Failed to lookup instruction " + SymbolicName).str().c_str());
}

I'm not sure how exactly it ends up concluding this is an instruction of some kind

The AArch64 and ARM changes look reasonable to me. @ab is this ok to continue?

aemerson accepted this revision.Sep 11 2019, 10:28 PM

LGTM. I think we should proceed with this given @ab's objection should be addressed now, and it's been half a year.

arsenm accepted this revision.Sep 18 2019, 6:31 PM

r372285

I can't close this for some reason

aemerson removed a reviewer: ab.Sep 19 2019, 1:32 PM

Removing Ahmed as reviewer so this can be closed.

This revision is now accepted and ready to land.Sep 19 2019, 1:32 PM
aemerson closed this revision.Sep 19 2019, 1:32 PM