Page MenuHomePhabricator

[SelectionDAG] computeKnownBits - support constant pool values from target
ClosedPublic

Authored by RKSimon on May 14 2019, 3:45 AM.

Details

Summary

This patch adds the overridable TargetLowering::getTargetConstantFromLoad function which allows targets to return any constant value loaded by a LoadSDNode node - only X86 makes use of this so far but everything should be in place for other targets.

computeKnownBits then uses this function to improve codegen, notably vector code after legalization.

A future commit will do the same for ComputeNumSignBits but computeKnownBits sees the bigger benefit.

This required a couple of fixes:

  • SimplifyDemandedBits must early-out for getTargetConstantFromLoad cases to prevent infinite loops of constant regeneration (similar to what we already do for BUILD_VECTOR).
  • Fix a AVX512 regression as we had trunc(shl(v8i32),v8i16) <-> shl(trunc(v8i16),v8i32) infinite loops after legalization.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.May 14 2019, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 3:45 AM
lebedev.ri added inline comments.
include/llvm/CodeGen/TargetLowering.h
3121 ↗(On Diff #199392)

'that will loaded'?

RKSimon updated this revision to Diff 199470.May 14 2019, 9:12 AM

Fix comment typo

craig.topper added inline comments.May 21 2019, 2:01 PM
lib/Target/X86/X86ISelLowering.cpp
5777 ↗(On Diff #199470)

Is there an implicit assumption here that constant pool loads won't be a SEXTLOAD/ZEXTLOAD/EXTLOAD? If they were we'd get a mismatch of bitwidth right?

Do we get any changes with only the computeKnownBits part of the patch (ie, without the demanded bits part of the patch)?

It seems ok, but I'm wondering if we can reduce risk and the test diffs by splitting those changes up.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2892 ↗(On Diff #199470)

its -> it's (or 'it is')

RKSimon marked an inline comment as done.May 22 2019, 1:30 AM

Do we get any changes with only the computeKnownBits part of the patch (ie, without the demanded bits part of the patch)?

The demanded bits part is necessary to stop an infinite loop (finds constant in load, creates new constant that gets lowered to a constant pool load, finds that constant in load, ....).

It seems ok, but I'm wondering if we can reduce risk and the test diffs by splitting those changes up.

They're dependent on each other - SimplifyDemandedBits has no way to know when a node is a constant already - we already have to do something similar for BUILD_VECTOR of constants.

I can pull out the DAGCombiner.cpp change as a pre-commit - but I don't think it will affect any codegen without the rest of the patch.

lib/Target/X86/X86ISelLowering.cpp
5777 ↗(On Diff #199470)

I think this can return a extension load constant - it's up to the caller to check the LoadSDNode - compute known bits explicitly only uses constants of the same bitwidth.

RKSimon updated this revision to Diff 200698.May 22 2019, 4:12 AM

rebase - added a comment warning that callers of getTargetConstantFromLoad must check for implicit extension

craig.topper added inline comments.May 22 2019, 9:58 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2892 ↗(On Diff #200698)

Would it be safer to check the extension type here too? Is it possible for the constant itself to be wider than the memory VT, but equal to the width of an extension such that this check would let it through?

RKSimon updated this revision to Diff 200786.May 22 2019, 10:30 AM

limit computeKnownBits to normal loads

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2892 ↗(On Diff #200698)

Limit to just isNormalLoad or isNON_EXTLoad?

RKSimon updated this revision to Diff 200921.May 23 2019, 3:37 AM

Use isNON_EXTLoad and pull out the Constant so we could add support for EXTLoad cases in the future if we see the need.

This revision is now accepted and ready to land.May 23 2019, 1:45 PM
This revision was automatically updated to reflect the committed changes.