-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[Hexagon] Add Hexagon Copy Hoisting pass #89313
Conversation
The purpose of this pass is to move the common copy instructions that are present in all the successor of a basic block (BB) to the end of BB. Co-authored-by: Jyotsna Verma <jverma@quicinc.com>
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
I was running an outdated version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments are primarily about modernizing this code. Please use range-for loops wherever possible.
|
||
public: | ||
static char ID; | ||
HexagonCopyHoisting() : MachineFunctionPass(ID), MFN(0), MRI(0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use nullptr
instead of 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
// Traverse through the basic blocks again and move the COPY instructions | ||
// that are present in all the successors of BB to BB. | ||
bool changed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout this file variable names start with a capital letter. Please keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// Traverse through the basic blocks again and move the COPY instructions | ||
// that are present in all the successors of BB to BB. | ||
bool changed = false; | ||
for (auto I = po_begin(&Fn), E = po_end(&Fn); I != E; ++I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to use range-for loops over iterator based loops. Most things in LLVM that have begin/end iterators also have an iterator range that you can use in a range-for loop.
Here specifically that would be for (MachineBasicBlock *BB : post_order(&Fn))
, then you'd need to use BB->
instead of BB.
in the body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
MachineFunction *MFN; | ||
MachineRegisterInfo *MRI; | ||
StringMap<MachineInstr *> CopyMI; | ||
std::vector<StringMap<MachineInstr *>> CopyMIList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more efficient to use a DenseMap
with the key being a pair of registers:
DenseMap<std::pair<Register, Register>, MachineInstr *>
but it's up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; done
for (auto BI = MFN->begin(), BE = MFN->end(); BI != BE; ++BI) { | ||
MachineBasicBlock *BB = &*BI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Range-for:
for (MachineBasicBlock &BB : *MFN)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
MachineInstr *otherMI = &*MII; | ||
for (MachineInstr::mop_iterator Mo = otherMI->operands_begin(), | ||
E = otherMI->operands_end(); | ||
Mo != E; ++Mo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
|
||
void HexagonCopyHoisting::moveCopyInstr(MachineBasicBlock *DestBB, | ||
StringRef key, MachineInstr *MI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
addMItoCopyList(MI); | ||
auto I = ++(DestBB->succ_begin()), E = DestBB->succ_end(); | ||
for (; I != E; ++I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (auto I = std::next(DestBB->succ_begin()), E = DestBB->end(); ++I)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
unsigned DstReg = MI->getOperand(0).getReg(); | ||
unsigned SrcReg = MI->getOperand(1).getReg(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MachineOperand::getReg()
now returns Register
, not unsigned
. Please see llvm/include/llvm/CodeGen/Register.h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
MRI->getRegClass(SrcReg) != &Hexagon::IntRegsRegClass) | ||
return; | ||
|
||
StringRef key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few final comments.
StringRef Key; | ||
SmallString<256> TmpData(""); | ||
(void)Twine(Register::virtReg2Index(DstReg)).toStringRef(TmpData); | ||
TmpData += '='; | ||
Key = Twine(Register::virtReg2Index(SrcReg)).toStringRef(TmpData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
#include "HexagonTargetMachine.h" | ||
#include "llvm/ADT/PostOrderIterator.h" | ||
#include "llvm/ADT/SmallString.h" | ||
#include "llvm/ADT/StringMap.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this include can be removed (replaced with include of DenseMap.h). DenseMap.h seems to be included indirectly by some other header file, but it's a good idea to include it directly (in case something changes in that other header file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
MachineFunction *MFN; | ||
MachineRegisterInfo *MRI; | ||
StringMap<MachineInstr *> CopyMI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@quic-pmacmurr Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Hello @quic-pmacmurr @iajbar and others, If you run the new testcase
with verifiers on like:
then you get a whole bunch of complaints:
(I noticed this when I ran lit tests after compiling with EXPENSIVE_CHECKS on. But as you see above you can reproduce just by adding -verify-machineinstrs to the llc command) |
Hi @mikaelholmen, thank you for bringing this to my attention. I have fixed the test case in a new PR: #90740 |
Adds the HexagonCopyHoisting pass, which moves a common copy instruction into a basic block if it is present in all successor basic blocks.