Skip to content

[Mips] Fix mfhi/mflo hazard miscompilation about div and mult #91449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2024
Merged

[Mips] Fix mfhi/mflo hazard miscompilation about div and mult #91449

merged 1 commit into from
Sep 23, 2024

Conversation

yingopq
Copy link
Contributor

@yingopq yingopq commented May 8, 2024 •
edited
Loading

Fix issue1: In mips1-4, require a minimum of 2 instructions between a mflo/mfhi and the next mul/dmult/div/ddiv/divu/ddivu instruction.
Fix issue2: In mips1-4, should not put mflo into the delay slot for the return.

Fix #81291

@yingopq yingopq force-pushed the Fix_bug_81291 branch 2 times, most recently from 225aaca to 4e7d114 Compare May 8, 2024 09:08
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@yingopq
Copy link
Contributor Author

yingopq commented May 9, 2024

@topperc @wzssyqa Could you help review? Thanks!

@wzssyqa wzssyqa self-requested a review May 9, 2024 08:46
@yingopq
Copy link
Contributor Author

yingopq commented May 24, 2024

Ping @wzssyqa

@yingopq
Copy link
Contributor Author

yingopq commented Jun 26, 2024

ping @wzssyqa @topperc, thanks!

@brad0
Copy link
Contributor

brad0 commented Jul 14, 2024

@wzssyqa @topperc Ping.

1 similar comment
@brad0
Copy link
Contributor

brad0 commented Jul 25, 2024

@wzssyqa @topperc Ping.

@yingopq
Copy link
Contributor Author

yingopq commented Aug 13, 2024

Hi @wzssyqa @topperc Could you help review? Thanks!

wzssyqa
wzssyqa reviewed Sep 10, 2024
View reviewed changes
llvm/lib/Target/Mips/MipsBranchExpansion.cpp Outdated
TII->insertNop(*(I->getParent()), std::next(I), I->getDebugLoc())
->bundleWithPred();
NumInsertedNops++;
if (Predicate(*I) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this condition is added here?
I guess you need some comment or description in commit msg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check again without this condition and add comment.

Copy link
Contributor Author

@yingopq yingopq Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted this condition and the test result of ./build/bin/clang --target=mipsel-freestanding -mcpu=mips3 -fomit-frame-pointer -S 4.c -o 1.s or with -O3 all are OK, I would update it. Maybe some reasons I added it now I forget. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I added a new function which makes it simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wzssyqa Could you help review, thanks!

llvm/lib/Target/Mips/MipsDelaySlotFiller.cpp Outdated
@@ -743,6 +743,12 @@ bool MipsDelaySlotFiller::searchRange(MachineBasicBlock &MBB, IterTy Begin,
bool InMicroMipsMode = STI.inMicroMipsMode();
const MipsInstrInfo *TII = STI.getInstrInfo();
unsigned Opcode = (*Slot).getOpcode();

if ((CurrI->getOpcode() == Mips::MFLO ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No MFHI here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just address this title issue and forgot % Modulo Operation. I would add it and related testcase.
Thanks for your review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@yingopq yingopq force-pushed the Fix_bug_81291 branch 2 times, most recently from 0e77c3d to bc9f787 Compare September 14, 2024 07:17
@yingopq
[Mips] Fix mfhi/mflo hazard miscompilation about div and mult
4852feb 
Fix issue1: In mips1-4, require a minimum of 2 instructions between
a mflo/mfhi and the next mul/dmult/div/ddiv/divu/ddivu instruction.
Fix issue2: In mips1-4, should not put mflo into the delay slot for
the return.

Fix llvm#81291
@wzssyqa wzssyqa merged commit 677177b into llvm:main Sep 23, 2024
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 23, 2024

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/6306

Here is the relevant piece of the build log for the reference 
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/4/11' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Users/buildbot/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests-LLVM-Unit-84521-4-11.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=11 GTEST_SHARD_INDEX=4 /Users/buildbot/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests
--

Script:
--
/Users/buildbot/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests --gtest_filter=FileSystemTest.permissions
--
Test Directory: /tmp/lit-tmp-8laalusv/file-system-test-5cf382
/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2325: Failure
Value of: CheckPermissions(fs::set_gid_on_exe)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2340: Failure
Value of: CheckPermissions(fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2347: Failure
Value of: CheckPermissions(fs::all_read | fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2350: Failure
Value of: CheckPermissions(fs::all_perms)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2355: Failure
Value of: CheckPermissions(fs::all_perms & ~fs::sticky_bit)
  Actual: false
Expected: true


/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2325
Value of: CheckPermissions(fs::set_gid_on_exe)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2340
Value of: CheckPermissions(fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2347
Value of: CheckPermissions(fs::all_read | fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
...

@DimitryAndric DimitryAndric mentioned this pull request Dec 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wzssyqa wzssyqa wzssyqa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[mips] mfhi/mflo hazard miscompilation
4 participants
@yingopq @brad0 @llvm-ci @wzssyqa