[llvm-commits] [llvm] r45022 - in /llvm/trunk: include/llvm/Target/TargetInstrInfo.h lib/Target/Target.td utils/TableGen/CodeGenInstruction.h utils/TableGen/CodeGenTarget.cpp utils/TableGen/InstrInfoEmitter.cpp

[Chris Lattner]

On Dec 13, 2007, at 5:48 PM, Bill Wendling wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=45022&view=rev
> Log:
> Add flags to indicate that there are "never" side effects or that  
> there "may be"
> side effects for machine instructions.

Hi Bill,

> +++ llvm/trunk/include/llvm/Target/TargetInstrInfo.h Thu Dec 13  
> 19:48:59 2007
> @@ -91,6 +91,18 @@
> // ARM instructions which can set condition code if 's' bit is set.
> const unsigned M_HAS_OPTIONAL_DEF      = 1 << 17;
>
> +// M_MAY_HAVE_SIDE_EFFECTS - Set if this instruction *might* have  
> side effects,
> +// e.g. load instructions. Note: This and M_NEVER_HAS_SIDE_EFFECTS  
> are mutually
> +// exclusive. You can't set both! If neither flag is set, then the  
> instruction
> +// *always* has side effects.
> +const unsigned M_MAY_HAVE_SIDE_EFFECTS = 1 << 18;
> +
> +// M_NEVER_HAS_SIDE_EFFECTS - Set if this instruction *never* has  
> side effects,
> +// e.g., xor on X86.  Note: This and M_MAY_HAVE_SIDE_EFFECTS are  
> mutually
> +// exclusive. You can't set both! If neither flag is set, then the  
> instruction
> +// *always* has side effects.
> +const unsigned M_NEVER_HAS_SIDE_EFFECTS = 1 << 19;

As others have pointed out, we need to be much more clear about what  
these mean.  Specifically, I'd list "NEVER" first (it is easier to  
explain). The pertinent point here is completely missing in the  
comments:  this flag is set on an instruction where there is a side  
effect that is not captured by any *operands* of the instruction or  
*other flags*.  Instructions that are "isBranch" instructions but have  
no other side effects should have M_NEVER_HAS_SIDE_EFFECTS set.  This  
flag should only be set on an instruction when *all instances* of an  
instruction of that opcode have no side effects in this way.

Your description of M_MAY_HAVE_SIDE_EFFECTS is also quite poor.  Its  
point in life is to handle the case when *some instances* of an  
instruction can have side effects, but others don't.  You need to  
explain that this causes the invocation of some virtual method (which  
should be named) that the target can implement to make the  
determination.  Give an example of why this is useful: loads in  
general have side effects, but loads from the constant pool don't.

This criticism just applies to the comments for the flags, the  
functionality of the patch looks great!

>   bit usesCustomDAGSchedInserter = 0; // Pseudo instr needing  
> special help.
>   bit hasCtrlDep   = 0;     // Does this instruction r/w ctrl-flow  
> chains?
>   bit isNotDuplicable = 0;  // Is it unsafe to duplicate this  
> instruction?
> +
> +  // Side effect flags - If neither of these flags is set, then the  
> instruction
> +  // *always* has side effects. Otherwise, it's one or the other.

Please also make these much more clear, as above.

Thanks Bill,

-Chris

> +  bit mayHaveSideEffects = 0;  // This instruction *may* have side  
> effects.
> +  bit neverHasSideEffects = 0; // This instruction never has side  
> effects.
>
>   InstrItinClass Itinerary = NoItinerary;// Execution steps used for  
> scheduling.
>
>
> Modified: llvm/trunk/utils/TableGen/CodeGenInstruction.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenInstruction.h?rev=45022&r1=45021&r2=45022&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/utils/TableGen/CodeGenInstruction.h (original)
> +++ llvm/trunk/utils/TableGen/CodeGenInstruction.h Thu Dec 13  
> 19:48:59 2007
> @@ -104,6 +104,8 @@
>     bool hasCtrlDep;
>     bool isNotDuplicable;
>     bool hasOptionalDef;
> +    bool mayHaveSideEffects;
> +    bool neverHasSideEffects;
>
>     /// ParseOperandName - Parse an operand name like "$foo" or  
> "$foo.bar",
>     /// where $foo is a whole operand and $foo.bar refers to a  
> suboperand.
>
> Modified: llvm/trunk/utils/TableGen/CodeGenTarget.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenTarget.cpp?rev=45022&r1=45021&r2=45022&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/utils/TableGen/CodeGenTarget.cpp (original)
> +++ llvm/trunk/utils/TableGen/CodeGenTarget.cpp Thu Dec 13 19:48:59  
> 2007
> @@ -395,9 +395,15 @@
>   usesCustomDAGSchedInserter = R- 
> >getValueAsBit("usesCustomDAGSchedInserter");
>   hasCtrlDep   = R->getValueAsBit("hasCtrlDep");
>   isNotDuplicable = R->getValueAsBit("isNotDuplicable");
> +  mayHaveSideEffects = R->getValueAsBit("mayHaveSideEffects");
> +  neverHasSideEffects = R->getValueAsBit("neverHasSideEffects");
>   hasOptionalDef = false;
>   hasVariableNumberOfOperands = false;
> -
> +
> +  if (mayHaveSideEffects && neverHasSideEffects)
> +    throw R->getName() +
> +      ": cannot have both 'mayHaveSideEffects' and  
> 'neverHasSideEffects' set!";
> +
>   DagInit *DI;
>   try {
>     DI = R->getValueAsDag("OutOperandList");
>
> Modified: llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp?rev=45022&r1=45021&r2=45022&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp Thu Dec 13  
> 19:48:59 2007
> @@ -253,8 +253,9 @@
>   if (Inst.hasOptionalDef) OS << "|M_HAS_OPTIONAL_DEF";
>   if (Inst.usesCustomDAGSchedInserter)
>     OS << "|M_USES_CUSTOM_DAG_SCHED_INSERTION";
> -  if (Inst.hasVariableNumberOfOperands)
> -    OS << "|M_VARIABLE_OPS";
> +  if (Inst.hasVariableNumberOfOperands) OS << "|M_VARIABLE_OPS";
> +  if (Inst.mayHaveSideEffects) OS << "|M_MAY_HAVE_SIDE_EFFECTS";
> +  if (Inst.neverHasSideEffects) OS << "|M_NEVER_HAS_SIDE_EFFECTS";
>   OS << ", 0";
>
>   // Emit all of the target-specific flags...
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits