Sergei Larin
2012-Oct-29 17:17 UTC
[LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
Jakob and anyone else who might be interested... Base on this patch back in August, I sense some need to double check with you whether it is OK to start making a heavy use of MachineOperand TargetFlags? We do seem to have a compelling reason for it in Hexagon, and I wanted to make sure that it is OK with everyone. I plan to use it for attributing target specific info to MOs and in more general case to MIs that those MOs belongs to. Part of my question would be - is it still unsafe to use it for MO_Register and if so, then why? Thanks a lot. Sergei --- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation> -----Original Message----- > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits- > bounces at cs.uiuc.edu] On Behalf Of Jakob Stoklund Olesen > Sent: Tuesday, August 28, 2012 1:06 PM > To: llvm-commits at cs.uiuc.edu > Subject: [llvm-commits] [llvm] r162770 - in /llvm/trunk: > include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp > > Author: stoklund > Date: Tue Aug 28 13:05:48 2012 > New Revision: 162770 > > URL: http://llvm.org/viewvc/llvm-project?rev=162770&view=rev > Log: > Don't allow TargetFlags on MO_Register MachineOperands. > > Register operands are manipulated by a lot of target-independent code, > and it is not always possible to preserve target flags. That means it > is not safe to use target flags on register operands. > > None of the targets in the tree are using register operand target > flags. > External targets should be using immediate operands to annotate > instructions with operand modifiers. > > Modified: > llvm/trunk/include/llvm/CodeGen/MachineOperand.h > llvm/trunk/lib/CodeGen/MachineInstr.cpp > > Modified: llvm/trunk/include/llvm/CodeGen/MachineOperand.h > URL: http://llvm.org/viewvc/llvm- > project/llvm/trunk/include/llvm/CodeGen/MachineOperand.h?rev=162770&r1> 162769&r2=162770&view=diff > ======================================================================> ======> --- llvm/trunk/include/llvm/CodeGen/MachineOperand.h (original) > +++ llvm/trunk/include/llvm/CodeGen/MachineOperand.h Tue Aug 28 > 13:05:48 > +++ 2012 > @@ -60,12 +60,15 @@ > /// union. > unsigned char OpKind; // MachineOperandType > > - /// SubReg - Subregister number, only valid for MO_Register. A > value of 0 > - /// indicates the MO_Register has no subReg. > - unsigned char SubReg; > - > - /// TargetFlags - This is a set of target-specific operand flags. > - unsigned char TargetFlags; > + // This union is discriminated by OpKind. > + union { > + /// SubReg - Subregister number, only valid for MO_Register. A > value of 0 > + /// indicates the MO_Register has no subReg. > + unsigned char SubReg; > + > + /// TargetFlags - This is a set of target-specific operand flags. > + unsigned char TargetFlags; > + }; > > /// IsDef/IsImp/IsKill/IsDead flags - These are only valid for > MO_Register > /// operands. > @@ -176,9 +179,17 @@ > /// > MachineOperandType getType() const { return > (MachineOperandType)OpKind; } > > - unsigned char getTargetFlags() const { return TargetFlags; } > - void setTargetFlags(unsigned char F) { TargetFlags = F; } > - void addTargetFlag(unsigned char F) { TargetFlags |= F; } > + unsigned char getTargetFlags() const { > + return isReg() ? 0 : TargetFlags; > + } > + void setTargetFlags(unsigned char F) { > + assert(!isReg() && "Register operands can't have target flags"); > + TargetFlags = F; > + } > + void addTargetFlag(unsigned char F) { > + assert(!isReg() && "Register operands can't have target flags"); > + TargetFlags |= F; > + } > > > /// getParent - Return the instruction that this operand belongs to. > > Modified: llvm/trunk/lib/CodeGen/MachineInstr.cpp > URL: http://llvm.org/viewvc/llvm- > project/llvm/trunk/lib/CodeGen/MachineInstr.cpp?rev=162770&r1=162769&r2 > =162770&view=diff > ======================================================================> ======> --- llvm/trunk/lib/CodeGen/MachineInstr.cpp (original) > +++ llvm/trunk/lib/CodeGen/MachineInstr.cpp Tue Aug 28 13:05:48 2012 > @@ -208,8 +208,8 @@ > hash_code llvm::hash_value(const MachineOperand &MO) { > switch (MO.getType()) { > case MachineOperand::MO_Register: > - return hash_combine(MO.getType(), MO.getTargetFlags(), > MO.getReg(), > - MO.getSubReg(), MO.isDef()); > + // Register operands don't have target flags. > + return hash_combine(MO.getType(), MO.getReg(), MO.getSubReg(), > + MO.isDef()); > case MachineOperand::MO_Immediate: > return hash_combine(MO.getType(), MO.getTargetFlags(), > MO.getImm()); > case MachineOperand::MO_CImmediate: > > > _______________________________________________ > llvm-commits mailing list > llvm-commits at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
Arnold Schwaighofer
2012-Oct-29 18:02 UTC
[LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
Hi Sergei, our use of target flags will be on immediate register operands if I am not mistaken (and if not we can always encode it as such)? I guess you are refering to the hexagon backend needing to distinguish between instances of an instruction that uses a constant value that can fit into the 4 byte of the instruction and one that encodes the immediate in an extra instruction slot (what we call a constant extended instruction). We can encode that with a target flag on the immediate machine operand. So the patch you quoted is not a problem. The quoted patch only prohibits the use of flags on register operands (that is MO.isReg()==true) because some of the code in CodeGen that deals with register operands does not appropriately handle target flags on the register operand (it might not transfer the information from an "old" operand to an "updated" operand). I guess I am asking you to clarify what you mean by heavy use of TargetFlags on MachineOperands. What specifically do you plan to do? Jakob, TargetFlags on immediate machine operands introduced as early as ISel Lowering should not be a problem, right? Thanks, Arnold On Mon, Oct 29, 2012 at 12:17 PM, Sergei Larin <slarin at codeaurora.org> wrote:> > Jakob and anyone else who might be interested... > > Base on this patch back in August, I sense some need to double check with > you whether it is OK to start making a heavy use of MachineOperand > TargetFlags? > We do seem to have a compelling reason for it in Hexagon, and I wanted to > make sure that it is OK with everyone. I plan to use it for attributing > target specific info to MOs and in more general case to MIs that those MOs > belongs to. > Part of my question would be - is it still unsafe to use it for > MO_Register and if so, then why? > > Thanks a lot. > > Sergei > > --- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by > The Linux Foundation > > >> -----Original Message----- >> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits- >> bounces at cs.uiuc.edu] On Behalf Of Jakob Stoklund Olesen >> Sent: Tuesday, August 28, 2012 1:06 PM >> To: llvm-commits at cs.uiuc.edu >> Subject: [llvm-commits] [llvm] r162770 - in /llvm/trunk: >> include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp >> >> Author: stoklund >> Date: Tue Aug 28 13:05:48 2012 >> New Revision: 162770 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=162770&view=rev >> Log: >> Don't allow TargetFlags on MO_Register MachineOperands. >> >> Register operands are manipulated by a lot of target-independent code, >> and it is not always possible to preserve target flags. That means it >> is not safe to use target flags on register operands. >> >> None of the targets in the tree are using register operand target >> flags. >> External targets should be using immediate operands to annotate >> instructions with operand modifiers. >> >> Modified: >> llvm/trunk/include/llvm/CodeGen/MachineOperand.h >> llvm/trunk/lib/CodeGen/MachineInstr.cpp >> >> Modified: llvm/trunk/include/llvm/CodeGen/MachineOperand.h >> URL: http://llvm.org/viewvc/llvm- >> project/llvm/trunk/include/llvm/CodeGen/MachineOperand.h?rev=162770&r1>> 162769&r2=162770&view=diff >> ======================================================================>> ======>> --- llvm/trunk/include/llvm/CodeGen/MachineOperand.h (original) >> +++ llvm/trunk/include/llvm/CodeGen/MachineOperand.h Tue Aug 28 >> 13:05:48 >> +++ 2012 >> @@ -60,12 +60,15 @@ >> /// union. >> unsigned char OpKind; // MachineOperandType >> >> - /// SubReg - Subregister number, only valid for MO_Register. A >> value of 0 >> - /// indicates the MO_Register has no subReg. >> - unsigned char SubReg; >> - >> - /// TargetFlags - This is a set of target-specific operand flags. >> - unsigned char TargetFlags; >> + // This union is discriminated by OpKind. >> + union { >> + /// SubReg - Subregister number, only valid for MO_Register. A >> value of 0 >> + /// indicates the MO_Register has no subReg. >> + unsigned char SubReg; >> + >> + /// TargetFlags - This is a set of target-specific operand flags. >> + unsigned char TargetFlags; >> + }; >> >> /// IsDef/IsImp/IsKill/IsDead flags - These are only valid for >> MO_Register >> /// operands. >> @@ -176,9 +179,17 @@ >> /// >> MachineOperandType getType() const { return >> (MachineOperandType)OpKind; } >> >> - unsigned char getTargetFlags() const { return TargetFlags; } >> - void setTargetFlags(unsigned char F) { TargetFlags = F; } >> - void addTargetFlag(unsigned char F) { TargetFlags |= F; } >> + unsigned char getTargetFlags() const { >> + return isReg() ? 0 : TargetFlags; >> + } >> + void setTargetFlags(unsigned char F) { >> + assert(!isReg() && "Register operands can't have target flags"); >> + TargetFlags = F; >> + } >> + void addTargetFlag(unsigned char F) { >> + assert(!isReg() && "Register operands can't have target flags"); >> + TargetFlags |= F; >> + } >> >> >> /// getParent - Return the instruction that this operand belongs to. >> >> Modified: llvm/trunk/lib/CodeGen/MachineInstr.cpp >> URL: http://llvm.org/viewvc/llvm- >> project/llvm/trunk/lib/CodeGen/MachineInstr.cpp?rev=162770&r1=162769&r2 >> =162770&view=diff >> ======================================================================>> ======>> --- llvm/trunk/lib/CodeGen/MachineInstr.cpp (original) >> +++ llvm/trunk/lib/CodeGen/MachineInstr.cpp Tue Aug 28 13:05:48 2012 >> @@ -208,8 +208,8 @@ >> hash_code llvm::hash_value(const MachineOperand &MO) { >> switch (MO.getType()) { >> case MachineOperand::MO_Register: >> - return hash_combine(MO.getType(), MO.getTargetFlags(), >> MO.getReg(), >> - MO.getSubReg(), MO.isDef()); >> + // Register operands don't have target flags. >> + return hash_combine(MO.getType(), MO.getReg(), MO.getSubReg(), >> + MO.isDef()); >> case MachineOperand::MO_Immediate: >> return hash_combine(MO.getType(), MO.getTargetFlags(), >> MO.getImm()); >> case MachineOperand::MO_CImmediate: >> >> >> _______________________________________________ >> llvm-commits mailing list >> llvm-commits at cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Jakob Stoklund Olesen
2012-Oct-29 18:11 UTC
[LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
On Oct 29, 2012, at 11:02 AM, Arnold Schwaighofer <arnolds at codeaurora.org> wrote:> Jakob, > TargetFlags on immediate machine operands introduced as early as ISel > Lowering should not be a problem, right?That should be fine. /jakob
Sergei Larin
2012-Oct-29 22:28 UTC
[LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
Arnold, I wanted to hear from Jacob is the original patch in question still needed, since our use of this field could surpass const extenders and could potentially include MO_Register. Jacob, Can you please comment? Thanks. Sergei --- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation> -----Original Message----- > From: Arnold Schwaighofer [mailto:arnolds at codeaurora.org] > Sent: Monday, October 29, 2012 1:02 PM > To: Sergei Larin > Cc: Jakob Stoklund Olesen; llvmdev at cs.uiuc.edu > Subject: Re: [LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: > include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp > > Hi Sergei, > > our use of target flags will be on immediate register operands if I am > not mistaken (and if not we can always encode it as such)? > > I guess you are refering to the hexagon backend needing to distinguish > between instances of an instruction that uses a constant value that can > fit into the 4 byte of the instruction and one that encodes the > immediate in an extra instruction slot (what we call a constant > extended instruction). > We can encode that with a target flag on the immediate machine operand. > So the patch you quoted is not a problem. > > The quoted patch only prohibits the use of flags on register operands > (that is MO.isReg()==true) because some of the code in CodeGen that > deals with register operands does not appropriately handle target flags > on the register operand (it might not transfer the information from an > "old" operand to an "updated" operand). > > I guess I am asking you to clarify what you mean by heavy use of > TargetFlags on MachineOperands. What specifically do you plan to do? > > Jakob, > TargetFlags on immediate machine operands introduced as early as ISel > Lowering should not be a problem, right? > > > Thanks, > Arnold > > On Mon, Oct 29, 2012 at 12:17 PM, Sergei Larin <slarin at codeaurora.org> > wrote: > > > > Jakob and anyone else who might be interested... > > > > Base on this patch back in August, I sense some need to double > check > > with you whether it is OK to start making a heavy use of > > MachineOperand TargetFlags? > > We do seem to have a compelling reason for it in Hexagon, and I > wanted > > to make sure that it is OK with everyone. I plan to use it for > > attributing target specific info to MOs and in more general case to > > MIs that those MOs belongs to. > > Part of my question would be - is it still unsafe to use it for > > MO_Register and if so, then why? > > > > Thanks a lot. > > > > Sergei > > > > --- > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > hosted by The Linux Foundation > > > > > >> -----Original Message----- > >> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits- > >> bounces at cs.uiuc.edu] On Behalf Of Jakob Stoklund Olesen > >> Sent: Tuesday, August 28, 2012 1:06 PM > >> To: llvm-commits at cs.uiuc.edu > >> Subject: [llvm-commits] [llvm] r162770 - in /llvm/trunk: > >> include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp > >> > >> Author: stoklund > >> Date: Tue Aug 28 13:05:48 2012 > >> New Revision: 162770 > >> > >> URL: http://llvm.org/viewvc/llvm-project?rev=162770&view=rev > >> Log: > >> Don't allow TargetFlags on MO_Register MachineOperands. > >> > >> Register operands are manipulated by a lot of target-independent > >> code, and it is not always possible to preserve target flags. That > >> means it is not safe to use target flags on register operands. > >> > >> None of the targets in the tree are using register operand target > >> flags. > >> External targets should be using immediate operands to annotate > >> instructions with operand modifiers. > >> > >> Modified: > >> llvm/trunk/include/llvm/CodeGen/MachineOperand.h > >> llvm/trunk/lib/CodeGen/MachineInstr.cpp > >> > >> Modified: llvm/trunk/include/llvm/CodeGen/MachineOperand.h > >> URL: http://llvm.org/viewvc/llvm- > >> > project/llvm/trunk/include/llvm/CodeGen/MachineOperand.h?rev=162770&r > >> 1> >> 162769&r2=162770&view=diff > >> > ====================================================================> >> => >> ======> >> --- llvm/trunk/include/llvm/CodeGen/MachineOperand.h (original) > >> +++ llvm/trunk/include/llvm/CodeGen/MachineOperand.h Tue Aug 28 > >> 13:05:48 > >> +++ 2012 > >> @@ -60,12 +60,15 @@ > >> /// union. > >> unsigned char OpKind; // MachineOperandType > >> > >> - /// SubReg - Subregister number, only valid for MO_Register. A > >> value of 0 > >> - /// indicates the MO_Register has no subReg. > >> - unsigned char SubReg; > >> - > >> - /// TargetFlags - This is a set of target-specific operand flags. > >> - unsigned char TargetFlags; > >> + // This union is discriminated by OpKind. > >> + union { > >> + /// SubReg - Subregister number, only valid for MO_Register. A > >> value of 0 > >> + /// indicates the MO_Register has no subReg. > >> + unsigned char SubReg; > >> + > >> + /// TargetFlags - This is a set of target-specific operand > flags. > >> + unsigned char TargetFlags; > >> + }; > >> > >> /// IsDef/IsImp/IsKill/IsDead flags - These are only valid for > >> MO_Register > >> /// operands. > >> @@ -176,9 +179,17 @@ > >> /// > >> MachineOperandType getType() const { return > >> (MachineOperandType)OpKind; } > >> > >> - unsigned char getTargetFlags() const { return TargetFlags; } > >> - void setTargetFlags(unsigned char F) { TargetFlags = F; } > >> - void addTargetFlag(unsigned char F) { TargetFlags |= F; } > >> + unsigned char getTargetFlags() const { > >> + return isReg() ? 0 : TargetFlags; } void > >> + setTargetFlags(unsigned char F) { > >> + assert(!isReg() && "Register operands can't have target > flags"); > >> + TargetFlags = F; > >> + } > >> + void addTargetFlag(unsigned char F) { > >> + assert(!isReg() && "Register operands can't have target > flags"); > >> + TargetFlags |= F; > >> + } > >> > >> > >> /// getParent - Return the instruction that this operand belongs > to. > >> > >> Modified: llvm/trunk/lib/CodeGen/MachineInstr.cpp > >> URL: http://llvm.org/viewvc/llvm- > >> > project/llvm/trunk/lib/CodeGen/MachineInstr.cpp?rev=162770&r1=162769& > >> r2 > >> =162770&view=diff > >> > ====================================================================> >> => >> ======> >> --- llvm/trunk/lib/CodeGen/MachineInstr.cpp (original) > >> +++ llvm/trunk/lib/CodeGen/MachineInstr.cpp Tue Aug 28 13:05:48 2012 > >> @@ -208,8 +208,8 @@ > >> hash_code llvm::hash_value(const MachineOperand &MO) { > >> switch (MO.getType()) { > >> case MachineOperand::MO_Register: > >> - return hash_combine(MO.getType(), MO.getTargetFlags(), > >> MO.getReg(), > >> - MO.getSubReg(), MO.isDef()); > >> + // Register operands don't have target flags. > >> + return hash_combine(MO.getType(), MO.getReg(), MO.getSubReg(), > >> + MO.isDef()); > >> case MachineOperand::MO_Immediate: > >> return hash_combine(MO.getType(), MO.getTargetFlags(), > >> MO.getImm()); > >> case MachineOperand::MO_CImmediate: > >> > >> > >> _______________________________________________ > >> llvm-commits mailing list > >> llvm-commits at cs.uiuc.edu > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Apparently Analagous Threads
- [LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
- [LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
- [LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp
- [LLVMdev] No more TargetFlags on MO_Register MachineOperands
- [LLVMdev] No more TargetFlags on MO_Register MachineOperands