Skip to content
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.

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

[NewPM][Hexagon] Add HexagonPassRegistry.def #86244

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

paperchalice
Copy link
Contributor

Prepare for dag-isel, also migrate some test case

Prepare for dag-isel, also migrate some test case
@paperchalice
Copy link
Contributor Author

The test case hvx-loopidiom-memcpy.ll may be invalid. There is no different between opt -march=hexagon -hexagon-loop-idiom -S and opt -mtriple=hexagon-- -p no-op-loop -S.

bool HexagonLoopIdiomRecognize::run(Loop *L) {
const Module &M = *L->getHeader()->getParent()->getParent();
if (Triple(M.getTargetTriple()).getArch() != Triple::hexagon)
return false;

The condition is always true when invoke opt -march=hexagon -hexagon-loop-idiom. 😕

@aeubanks
Copy link
Contributor

yeah it does seem like we should remove that check. the pass should only be added for hexagon backends anyway

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

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

the existing tests don't seem to great, but this patch on its own is good

@@ -1,4 +1,4 @@
; RUN: opt -S -hexagon-loop-idiom < %s | opt -S -passes='loop(loop-deletion),gvn'
; RUN: opt -mtriple hexagon-- -S -passes='loop(hexagon-loop-idiom,loop-deletion),gvn'
; REQUIRES: asserts
Copy link
Contributor

Choose a reason for hiding this comment

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

this REQUIRES doesn't make sense, we should run it under all configurations...

same for the other tests here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this test should be run under all configurations. REQUIRES should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, but should be done in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll push a patch to remove REQUIRES.

@@ -1,4 +1,4 @@
; RUN: opt -S -hexagon-loop-idiom < %s | opt -S -passes='loop(loop-deletion),gvn'
; RUN: opt -mtriple hexagon-- -S -passes='loop(hexagon-loop-idiom,loop-deletion),gvn'
; REQUIRES: asserts

; This tests that the HexagonLoopIdiom pass does not mark LCSSA information
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an issue. the new PM loop infra assumes that loops are always in LCSSA form. either the pass needs to ensure the loops are in LCSSA form, or perhaps it can just call a helper method

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll push a patch to make sure LCSSA form is preserved.

@@ -1,4 +1,5 @@
; RUN: opt -march=hexagon -hexagon-loop-idiom -S < %s | FileCheck %s
; RUN: opt -march=hexagon -p hexagon-loop-idiom -S < %s | FileCheck %s
; REQUIRES: asserts
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Please remove REQUIRES.

@paperchalice paperchalice merged commit ef57977 into llvm:main Mar 23, 2024
4 checks passed
@paperchalice paperchalice deleted the hexagon-registry branch March 23, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants