[LLVMdev] [llvm-commits] [llvm] r162770 - in /llvm/trunk: include/llvm/CodeGen/MachineOperand.h lib/CodeGen/MachineInstr.cpp

[Arnold Schwaighofer]

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