This is an archive of the discontinued LLVM Phabricator instance.

[HardwareLoops] NFC - move hardware loop checking code to isHardwareLoopProfitable()
ClosedPublic

Authored by shchenz on Jul 4 2019, 1:38 AM.

Details

Summary

Move more hardware checking codes into isHardwareLoopProfitable():
1: a more accurate result about hardware loop converting for target hook canSaveCmp() (Currently only used in PowerPC)
2: counter NumHWLoops is also more accurate.

This is a NFC patch.

Diff Detail

Event Timeline

shchenz created this revision.Jul 4 2019, 1:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2019, 1:38 AM
shchenz edited reviewers, added: steven.zhang; removed: qshanz.Jul 4 2019, 1:44 AM
samparker added inline comments.Jul 9 2019, 2:26 AM
llvm/lib/Analysis/TargetTransformInfo.cpp
157

Looks like this logic can be simplified now as UseLoopGuard is only ever equal to ForceGuardLoopEntry.

shchenz updated this revision to Diff 208647.Jul 9 2019, 5:15 AM

address @samparker comments.

shchenz marked 2 inline comments as done.Jul 9 2019, 5:17 AM
shchenz added inline comments.
llvm/lib/Analysis/TargetTransformInfo.cpp
157

Good idea. Get your point. No matter 'test and set' form may fallback to the 'set' form or not, it will not impact that the Loop is a hardware loop. Updated accordingly.

samparker accepted this revision.Jul 9 2019, 7:21 AM

Thanks, LGTM

This revision is now accepted and ready to land.Jul 9 2019, 7:21 AM
This revision was automatically updated to reflect the committed changes.
shchenz marked an inline comment as done.

I am not sure if you've seen this yet, but this broke the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/6478

sbc100 added a subscriber: sbc100.Jul 9 2019, 10:05 AM

This broke my build too. Could be related to the fact that I build with -DBUILD_SHARED_LIBS=ON?

This added an extra dependency from Analysis -> TransformUtils. I don't think this dependency is a good idea, so is adding preheader in analysis pass.

I tried adding a dependency on TransformUtils to Analysis/LLVMBuild.txt but that generated:

CMake Error at CMakeLists.txt:711 (message):
  Unexpected failure executing llvm-build: llvm-build: fatal error: found
  cycle to u'Analysis' after following: u'AArch64CodeGen' (required library)
  -> u'Analysis' (required library) -> u'TransformUtils' (required library)
  -> Analysis

Does this mean that this change violates the dependency graph? Assuming you can repro, with -DBUILD_SHARED_LIBS=ON then perhaps we can revert this while a solution is found?

This revision violates some rules about the build structure.

In particular, Analysis shouldn't depend on Transforms, and the new includes in TargetTransform.cpp make it do so.

These headers would have to be moved into the includes directory for this to be a viable change.

One possible fix would be to move TargetTransform.cpp from Analysis into Transform/Utils.

I propose we that we revert this revision.

I tried adding a dependency on TransformUtils to Analysis/LLVMBuild.txt but that generated:

CMake Error at CMakeLists.txt:711 (message):
  Unexpected failure executing llvm-build: llvm-build: fatal error: found
  cycle to u'Analysis' after following: u'AArch64CodeGen' (required library)
  -> u'Analysis' (required library) -> u'TransformUtils' (required library)
  -> Analysis

Does this mean that this change violates the dependency graph? Assuming you can repro, with -DBUILD_SHARED_LIBS=ON then perhaps we can revert this while a solution is found?

Between modifying the CFG in an analysis pass and breaking the shared libs build, I think reverting for now is a good idea.

Thanks for reverting @jsji . I will see how to fix it.