This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Small refactoring changes, NFC
ClosedPublic

Authored by kparzysz on Sep 15 2020, 6:58 AM.

Details

Summary
  1. Store intrinsic ID in ParseMemoryInst instead of a boolean flag IsTargetMemInst. This will make it easier to add support for target-independent intrinsics.
  2. Extract the complex multiline conditions from EarlyCSE::processNode into a new function getMatchingValue.

Diff Detail

Event Timeline

kparzysz created this revision.Sep 15 2020, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 6:58 AM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript

Mostly looks fine; couple minor suggestions.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
983

Is InValFirst the same thing as !MemInst.isLoad()? Is there some useful semantic difference I'm missing?

1294

Unnecessary InVal.DefInst &&?

kparzysz marked 2 inline comments as done.Sep 18 2020, 8:03 AM
kparzysz added inline comments.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1294

The lookup can return an "empty" LoadValue, one that has DefInst set to nullptr. In that case, getMatchingValue will also return nullptr, so we need this check.

kparzysz updated this revision to Diff 292793.Sep 18 2020, 8:09 AM
kparzysz marked an inline comment as done.

Removed the bool argument to getMatchingValue. Added comments explaining how the return value is used.

This revision is now accepted and ready to land.Sep 21 2020, 1:53 PM
This revision was landed with ongoing or failed builds.Sep 21 2020, 2:11 PM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Sep 21 2020, 3:02 PM

FWIW there were code size changes with this patch, so it wasn't NFC.

fhahn added a subscriber: fhahn.Sep 23 2020, 2:23 AM

I think it looks like this patch is causing EarlyCSE to crash with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17662/consoleFull#-261651283a1ca8a51-895e-46c6-af87-ce24fa4cd561. This commit is the only CSE/MemorySSA related change in the build that started crashing.

It would be great if you could take a look or revert if it takes longer to fix.

It would be great if you could take a look or revert if it takes longer to fix.

Sure, let me check.

It would be great if you could take a look or revert if it takes longer to fix.

Sure, let me check.

Fix is coming soon.