This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Extend SearchForAndLoads with any_extend handling
ClosedPublic

Authored by dmgreen on Jan 17 2022, 12:42 AM.

Details

Summary

This extends the code in SearchForAndLoads to be able to look through ANY_EXTEND nodes, which can be created from mismatching IR types where the AND node we begin from only demands the low parts of the register. That turns zext and sext into any_extends as only the low bits are demanded. To be able to look through ANY_EXTEND nodes we need to handle mismatching types in a few places, potentially truncating the mask to the size of the final load.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 17 2022, 12:42 AM
dmgreen requested review of this revision.Jan 17 2022, 12:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2022, 12:42 AM
This revision is now accepted and ready to land.Jan 17 2022, 12:57 AM
RKSimon accepted this revision.Jan 17 2022, 5:53 AM

X86 change LGTM

This revision was landed with ongoing or failed builds.Jan 17 2022, 7:25 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jan 18 2022, 1:54 AM

This is causing builds to fail with an assert:

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5638: bool (anonymous namespace)::DAGCombiner::BackwardsPropagateMask(llvm::SDNode *): Assertion `NewLoad && "Shouldn't be masking the load if it can't be narrowed"' failed.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1288202#c1 for a reproducer.

I'll revert this until it can be fixed.

dmgreen added a comment.EditedJan 18 2022, 1:19 PM

Thanks for the report. I've recommitted with a fix. Please let me know if there are any other issues that come up.

jonpa added a subscriber: jonpa.Feb 1 2022, 11:03 AM

Unfortunately I think this is causing wrong code: https://github.com/llvm/llvm-project/issues/53528

Oh I see yes. That is essentially the same as the multiple_load_or case here - I was trying to get that to go wrong but couldn't. On AArch64 we implicitly zext (in the orr w0, w9, w8, the top bits of x0 are set to 0 as it writes w0). In the case you have it doesn't get the zext.

I will try and sort out a fix, and make sure this is either fixed or reverted on the branch (depending on which revision that gets created from).