-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[PowerPC] redesign the target flags #69695
Conversation
65b8bf1
to
8dad10a
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
8dad10a
to
d313dfd
Compare
'Target flags' is confusing considering content of this patch. Maybe 'target operand flags'? |
ping |
llvm/lib/Target/PowerPC/PPC.h
Outdated
/// On PPC, the 12 bits are not enough for all target operand flags. | ||
/// Treat all PPC target flags as direct flags. This also means we can not | ||
/// use a bitmask flag, so if one operand has two or more flags, a fake | ||
/// combination flag must be created. See example MO_GOT_TPREL_PCREL_FLAG. | ||
|
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.
/// On PPC, 12 bits are not enough as orthogonal masks. Define flags in
/// sequence to save space. To define new flag, add new enum entry instead
/// of combining existing flags. See MO_GOT_TPREL_PCREL_FLAG as example.
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 orthogonal masks
, do you mean bitmask flag? Actually 12 bits are for both "bitmask" flag and "direct flag". Before the change, we have 8 orthogonal mask and 4 bit for direct mask.
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.
Yes, I did not catch meaning of direct flag when I wrote the comment. Is fake combination flag
still confusing though? Maybe ... a new direct flag must be created ...
.
@@ -830,7 +816,9 @@ void PPCAsmPrinter::emitInstruction(const MachineInstr *MI) { | |||
// For TLS initial-exec and local-exec accesses on AIX, we have one TOC | |||
// entry for the symbol (with the variable offset), which is differentiated | |||
// by MO_TPREL_FLAG. | |||
if (MO.getTargetFlags() & PPCII::MO_TPREL_FLAG) { | |||
if (MO.getTargetFlags() == PPCII::MO_TPREL_FLAG || |
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 may be good to pull out MO.getTargetFlags()
since we access it multiple times here.
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
@@ -1539,7 +1528,9 @@ void PPCAsmPrinter::emitInstruction(const MachineInstr *MI) { | |||
// with an immediate operand having the MO_TPREL_FLAG. Such an instruction | |||
// does not otherwise arise. | |||
const MachineOperand &MO = MI->getOperand(2); | |||
if ((MO.getTargetFlags() & PPCII::MO_TPREL_FLAG) != 0) { | |||
if (MO.getTargetFlags() == PPCII::MO_TPREL_FLAG || |
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.
Perhaps the same here, we could pull out MO.getTargetFlags()
due to multiple accesses.
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
Thanks for review @ecnelises @amy-kwan . Comments addressed. |
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 nits.
/// Symbol for VK_PPC_TLS fixup attached to an ADD instruction | ||
MO_TLS, | ||
|
||
/// MO_PIC_HA_FLAG = MO_PIC_FLAG | MO_HA |
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 comment looks confusing, since they were previously mask ORed but now are not. Can you please provide full description?
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.
Instead of adding comments for each mask that are combination of other maks, I added a comment in the very begining about how to add a new mask.
Redesign how PPC target set the target specific flags. With this patch, all ppc target flags are direct flags. No bitmask flag in PPC anymore. 12 bit is not enough for PPC's target specific flags. If 8 bit for the bitmask flags, 4 bit for the direct mask, PPC can total have 16 direct mask and 8 bitmask. Not enough for PPC, see this issue in #66316
Reducing the bitmask's 8 bit to a smaller value would be also workable, but that is not robust if we continue to add more target specific flags.
Increasing the size of
SubReg_TargetFlags
inMachineOperand
is also workable, but that will increase memory usage for all targets. I think this should be avoided.This patch aligns with some targets like X86 which also has many target specific flags.
The patch also fixes a bug related to flag
MO_TLSGDM_FLAG
andMO_LO
. They are the same value and the test case changes in this PR shows the bug.