This is an archive of the discontinued LLVM Phabricator instance.

[Hexagon] Make HexagonVLCR compatibile with New PM
ClosedPublic

Authored by quic_aankit on Sep 1 2020, 9:33 AM.

Details

Summary

The patch modifies HexagonVectorLoopCarriedReuse pass to make it compatible with both Legacy Pass Manager through HexagonVectorLoopCarriedReuseLegacyPass and with New Pass Manager through HexagonVectorLoopCarriedReusePass.

Diff Detail

Event Timeline

quic_aankit created this revision.Sep 1 2020, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 9:33 AM
quic_aankit requested review of this revision.Sep 1 2020, 9:33 AM
pzheng added a comment.Sep 1 2020, 4:51 PM

Is there any test case for this pass? Please also run clang-format.

llvm/lib/Target/Hexagon/HexagonVectorLoopCarriedReuse.cpp
279

Is this removed intentionally?

quic_aankit added inline comments.Sep 3 2020, 2:39 PM
llvm/lib/Target/Hexagon/HexagonVectorLoopCarriedReuse.cpp
279

Yeah. I did find it being used anywhere

Ran clang-format on the patch

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Hi pzheng, I could not figure out a way to register a target specific pass in opt? I can see that llvm/lib/Passes/PassRegistry.def is the registry for target independent passes. Can you point me to an example/API which can help me in registering this pass so that I can use it -passes option in opt?

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Hi pzheng, I could not figure out a way to register a target specific pass in opt? I can see that llvm/lib/Passes/PassRegistry.def is the registry for target independent passes. Can you point me to an example/API which can help me in registering this pass so that I can use it -passes option in opt?

I am not sure if there is a better place to register the pass other than PassRegistry.def. Maybe someone who knows can comment on this.

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Hi pzheng, I could not figure out a way to register a target specific pass in opt? I can see that llvm/lib/Passes/PassRegistry.def is the registry for target independent passes. Can you point me to an example/API which can help me in registering this pass so that I can use it -passes option in opt?

I am not sure if there is a better place to register the pass other than PassRegistry.def. Maybe someone who knows can comment on this.

Hi pzheng, testing target specific passes using opt+NPM is not currently supported and the work is still work-in-progress. Can we land this without modifying the testcases for now?

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Hi pzheng, I could not figure out a way to register a target specific pass in opt? I can see that llvm/lib/Passes/PassRegistry.def is the registry for target independent passes. Can you point me to an example/API which can help me in registering this pass so that I can use it -passes option in opt?

I am not sure if there is a better place to register the pass other than PassRegistry.def. Maybe someone who knows can comment on this.

Hi pzheng, testing target specific passes using opt+NPM is not currently supported and the work is still work-in-progress. Can we land this without modifying the testcases for now?

Hi @quic_aankit, just to be clear, you tried registering the pass in PassRegistry.def, but still couldn't get "opt -passes=hexagon-vlcr" to work, right? If so, I am fine with leaving out the tests for now, but we should probably mention in the commit message that tests are still to be ported.

Is there any existing test case for this pass with the legacy pass manager? If so, you might want to add one with the new pass manager too.

All the existing testcases use opt to run the pass. The pass is not run integrated in the main list of passes for any -Ox level.

That's what I meant. For each legacy pass manager test "RUN: opt -hexagon-vlcr ...", you can add a corresponding new pass manager test "RUN: opt -passes=hexagon-vlcr". This way, we will have the same test coverage with both the legacy and new pass managers.

Hi pzheng, I could not figure out a way to register a target specific pass in opt? I can see that llvm/lib/Passes/PassRegistry.def is the registry for target independent passes. Can you point me to an example/API which can help me in registering this pass so that I can use it -passes option in opt?

I am not sure if there is a better place to register the pass other than PassRegistry.def. Maybe someone who knows can comment on this.

Hi pzheng, testing target specific passes using opt+NPM is not currently supported and the work is still work-in-progress. Can we land this without modifying the testcases for now?

Hi @quic_aankit, just to be clear, you tried registering the pass in PassRegistry.def, but still couldn't get "opt -passes=hexagon-vlcr" to work, right? If so, I am fine with leaving out the tests for now, but we should probably mention in the commit message that tests are still to be ported.

Yes @pzheng . I cannot register the pass the in PassRegisry.def as I cannot include the requried header file from lib/target/hexagon. The LLVM community confirmed that testing target specific passes using opt is a work in progress and not supported yet.

pzheng accepted this revision.Sep 18 2020, 5:02 PM
This revision is now accepted and ready to land.Sep 18 2020, 5:02 PM
This revision was automatically updated to reflect the committed changes.