This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Hardware Loop branch instruction's condition may not be icmp
ClosedPublic

Authored by shchenz on Jul 3 2019, 2:15 AM.

Details

Summary

This is for https://bugs.llvm.org/show_bug.cgi?id=42492

In patch for https://reviews.llvm.org/D63477, it assumes that all HardwareLoops loop branch instruction's condition is a icmp. This should be wrong.

For case in https://bugs.llvm.org/show_bug.cgi?id=42492 , branch instruction's condition is not a icmp, it is a or instead.

Diff Detail

Repository
rL LLVM

Event Timeline

shchenz created this revision.Jul 3 2019, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 2:15 AM

Please could you add an opt test for the hardware-loops too? This looks like a corner case that we're not getting generic coverage on.

shchenz updated this revision to Diff 207770.Jul 3 2019, 5:46 AM

@samparker Thanks for your comment, Sam. Updated the patch. I use the same source in both opt test and llc test, since the testing point is not the same, hope this is ok.

samparker accepted this revision.Jul 3 2019, 9:16 AM

Thanks. LGTM

This revision is now accepted and ready to land.Jul 3 2019, 9:16 AM

Thanks. LGTM

LGTM too, just some comments about the test.

llvm/test/CodeGen/PowerPC/pr42492.ll
1 ↗(On Diff #207770)

You don't need 'REQUIRES: asserts' here. 'REQUIRES: asserts' is only necessary if asserts are necessary for the test to pass, not if asserts is required for the test to fail. The test still might fail anyway (e.g., it might segfault).

Also, in this case, it seems as though the test can test for correct behavior (not just the absence of a crash), and so please add some CHECK lines to check for the expected output in this case.

shchenz updated this revision to Diff 207960.Jul 3 2019, 6:47 PM

address review comments about test case.

This revision was automatically updated to reflect the committed changes.