This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Rewrite areNonVolatileConsecutiveLoads to use BaseIndexOffset
ClosedPublic

Authored by niravd on Jun 21 2017, 12:56 PM.

Details

Summary

As discussed in D34087, rewrite areNonVolatileConsecutiveLoads using
generic check. Also, propagate address handling to BaseIndexOffset.

Effectively this copies address analysis logic from DAGCombiner's isAlias to areNonVolatileConsecutiveLoads and vice versa:

Test changes due to areNonVolatileConsecutiveLoads improvements:

  • test/CodeGen/X86/build-vector-{128,256,512} - Missed Merges now occur

Test changes due improvements in DAGCombiner alias analysis

  • test/CodeGen/BPF/undef.ll - Handling of constant OR-based address allows an additional store merge
  • test/CodeGen/X86/clear_upper_vector_element_bits.ll - This is a case we already do not handle well. Here, the DAG is improved, but scheduling causes a code size degradation.

Event Timeline

niravd created this revision.Jun 21 2017, 12:56 PM
RKSimon edited edge metadata.Jun 22 2017, 3:06 AM
  • test/CodeGen/X86/clear_upper_vector_element_bits.ll - DAG is improved, but scheduling causes a code size degradation. This is probably okay.

That's alright - that test is a rather convoluted case, at some point the build vector lowering should be able to avoid it properly.

Some minor comments.

lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
31

This can be done as an NFC commit to trunk now - it's not dependent on the patch.

76

This LHS test shouldn't be needed - constants in ADD/OR should have been canonicalized to the RHS.

test/CodeGen/MSP430/Inst16mm.ll
67

If these are DAG checks would it better to replace the hard coded registers with regex?

test/CodeGen/PowerPC/complex-return.ll
12 ↗(On Diff #103444)

Should this be done as a pre-commit?

niravd edited the summary of this revision. (Show Details)Jun 23 2017, 7:02 PM
niravd updated this revision to Diff 103825.Jun 23 2017, 7:03 PM
niravd marked an inline comment as done.
niravd edited the summary of this revision. (Show Details)

Commit complex-return.ll test separately.

niravd edited the summary of this revision. (Show Details)Jun 24 2017, 7:53 PM
niravd updated this revision to Diff 104337.Jun 27 2017, 7:15 PM
niravd edited the summary of this revision. (Show Details)

Rebase after NFC FrameIndex to BaseIndexOffset.

niravd marked an inline comment as done.Jun 27 2017, 7:18 PM
niravd marked an inline comment as not done.
pftbest added inline comments.
test/CodeGen/MSP430/Inst16mm.ll
67

I think this test checks that we do direct memory to memory transfer without loading into intermediate register. r1 is a stack pointer, so it's fixed and cannot change.

What can change is the order of this two movs. Is there some CHECK keyword that ignores the order of instructions?

RKSimon added inline comments.Jun 28 2017, 3:20 AM
lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
76

You still have both of these here, but removing the Base->getOperand(0) doesn't seem to have any effect on the tests. Do you have cases where Base->getOperand(0) is constant?

test/CodeGen/MSP430/Inst16mm.ll
67

Yes CHECK-DAG should do that. If we're guaranteed to get r1 then we don't need a regex - thanks for the clarification.

niravd added inline comments.Jun 28 2017, 8:22 AM
lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
76

I don't. This was added to match with isGAPlusOffset which was rewritten similarly in a previous patch which I've dropped for now. I'm not sure if that's actually interesting. Regardless, I'll remove this here and defer that until that patch appears.

niravd updated this revision to Diff 104700.Jun 29 2017, 11:20 AM

Resolve outstanding comments and rebase of landed extracted NFC patch

RKSimon accepted this revision.Jun 29 2017, 11:29 AM

LGTM - thanks

This revision is now accepted and ready to land.Jun 29 2017, 11:29 AM
This revision was automatically updated to reflect the committed changes.
niravd reopened this revision.Jun 30 2017, 6:02 AM

Apparently there's a failure in stage1 builds on ppc64be machines where store merge constants are being constructed in byte reversed.

This revision is now accepted and ready to land.Jun 30 2017, 6:02 AM

Apparently there's a failure in stage1 builds on ppc64be machines where store merge constants are being constructed in byte reversed.

Was this reported offline or is there a buildbot log that shows the problem?

This revision was automatically updated to reflect the committed changes.