[x86 - inline asm] Favor reg over memory in asm w/ one input parameter

[Gerolf]

Summary

When more than one location for an asm parameter is specified,
the most general is selected. In the specific case of an 'rm' constraint
this gives higher priority to a memory vs register location.
In case there is only one input operand this is too conservative.
The purpose of this patch is to allow higher priority for the
register location in that case.

[echristo]

Hi Gerolf,

I'm not sure this is the right way to go. What I'm assuming (since you don't mention) the case that you have is something where the operand is actually already in a register and you want to just go ahead and use that rather than picking the alternative that copies it to memory? Why not solve that problem by looking through the various possible constraints and if the operand you want is already in a location then pick that and worry about forcing as separate?

-eric

[Gerolf]

Hi Eric

yes, there are more general ways of handling this, and for a single operand your suggestion improves on the implementation I currently have. Wrt to the more general solution it seems that it could result in a problem - at least when implemented naively like for one operand at a time - e.g. when an operand resides in a location needed by another one. This is indicated by a comment in ChooseConstraint() " Ideally, we would pick the most specific constraint possible: if we have something that fits into a register, we would pick it. The problem here is that if we have something that could either be in a register or in memory that use of the register could cause selection of *other* operands to fail: they might only succeed if we pick memory. ".
The most general route for handling this would be to enhance cg copy prop to handle cases like vreg->mem etc. But that is more longer term and would be an overkill for the isolated instance at hand.
So I think I go ahead and add a location check to my current code. Would that stand a chance to pass your LGTM threshold?

Cheers
Gerolf

[Gerolf]

Only give register preference to the parameter when it resides in a register as Eric suggested.

include/llvm/Target/TargetLowering.h

public:
   /// The operand object must already have been set up with the operand type.
   virtual ConstraintWeight getSingleConstraintMatchWeight(
       AsmOperandInfo &info, const char *constraint) const;
 
   /// Determines the constraint code and constraint type to use for the specific
   /// AsmOperandInfo, setting OpInfo.ConstraintCode and OpInfo.ConstraintType.
   /// If the actual operand being passed in is available, it can be passed in as
   /// Op, otherwise an empty SDValue can be passed.
-  virtual void ComputeConstraintToUse(AsmOperandInfo &OpInfo,
-                                      SDValue Op,
-                                      SelectionDAG *DAG = nullptr) const;
+  /// When \param PreferReg is true prefer register over memory.
+  virtual void ComputeConstraintToUse(AsmOperandInfo &OpInfo, SDValue Op,
+                                      SelectionDAG *DAG = nullptr,
+                                      bool PreferReg = false) const;
 
   /// Given a constraint, return the type of constraint it is for this target.
   virtual ConstraintType getConstraintType(const std::string &Constraint) const;
 
   /// Given a physical register constraint (e.g.  {edx}), return the register
   /// number and the register class for the register.
   ///
   /// Given a register class constraint, like 'r', if this corresponds directly

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitInlineAsm(ImmutableCallSite CS) {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   TargetLowering::AsmOperandInfoVector TargetConstraints =
       TLI.ParseConstraints(DAG.getSubtarget().getRegisterInfo(), CS);
 
   bool hasMemory = false;
 
   unsigned ArgNo = 0;   // ArgNo - The argument of the CallInst.
   unsigned ResNo = 0;   // ResNo - The result number of the next output.
+  unsigned NumInputOperands = 0; // #Input operands.
+  unsigned InputInVreg = 0;
   for (unsigned i = 0, e = TargetConstraints.size(); i != e; ++i) {
     ConstraintOperands.push_back(SDISelAsmOperandInfo(TargetConstraints[i]));
     SDISelAsmOperandInfo &OpInfo = ConstraintOperands.back();
 
     MVT OpVT = MVT::Other;
 
     // Compute the value type for each operand.
     switch (OpInfo.Type) {
case InlineAsm::isOutput:
       if (StructType *STy = dyn_cast<StructType>(CS.getType())) {
         OpVT = TLI.getSimpleValueType(STy->getElementType(ResNo));
       } else {
         assert(ResNo == 0 && "Asm only has one result!");
         OpVT = TLI.getSimpleValueType(CS.getType());
       }
       ++ResNo;
       break;
-    case InlineAsm::isInput:
+    case InlineAsm::isInput: {
       OpInfo.CallOperandVal = const_cast<Value *>(CS.getArgument(ArgNo++));
-      break;
+      DenseMap<const Value *, unsigned>::iterator VMI =
+          FuncInfo.ValueMap.find(OpInfo.CallOperandVal);
+      if (VMI != FuncInfo.ValueMap.end()) {
+        InputInVreg = 1;
+      }
+      ++NumInputOperands;
+    } break;
     case InlineAsm::isClobber:
       // Nothing to do.
       break;
     }
 
     // If this is an input or an indirect output, process the call argument.
     // BasicBlocks are labels, currently appearing only in asm's.
     if (OpInfo.CallOperandVal) {
const TargetRegisterInfo *TRI = DAG.getSubtarget().getRegisterInfo();
                              " with a matching output constraint of"
                              " incompatible type!");
         }
         Input.ConstraintVT = OpInfo.ConstraintVT;
       }
     }
 
     // Compute the constraint code and ConstraintType to use.
-    TLI.ComputeConstraintToUse(OpInfo, OpInfo.CallOperand, &DAG);
+    TLI.ComputeConstraintToUse(OpInfo, OpInfo.CallOperand, &DAG,
+                               OpInfo.Type == InlineAsm::isInput &&
+                                   NumInputOperands == 1 && InputInVreg == 1);
 
     if (OpInfo.ConstraintType == TargetLowering::C_Memory &&
         OpInfo.Type == InlineAsm::isClobber)
       continue;
 
     // If this is a memory input, and if the operand is not indirect, do what we
     // need to to provide an address for the memory input.
     if (OpInfo.ConstraintType == TargetLowering::C_Memory &&

lib/CodeGen/SelectionDAG/TargetLowering.cpp

 /// memory that use of the register could cause selection of *other*
 /// operands to fail: they might only succeed if we pick memory.  Because of
 /// this the heuristic we use is:
 ///
 ///  1) If there is an 'other' constraint, and if the operand is valid for
 ///     that constraint, use it.  This makes us take advantage of 'i'
 ///     constraints when available.
 ///  2) Otherwise, pick the most general constraint present.  This prefers
-///     'm' over 'r', for example.
+///     'm' over 'r', for example. But this can be overruled by \param
+/// PreferReg.
 ///
 static void ChooseConstraint(TargetLowering::AsmOperandInfo &OpInfo,
-                             const TargetLowering &TLI,
-                             SDValue Op, SelectionDAG *DAG) {
+                             const TargetLowering &TLI, SDValue Op,
+                             SelectionDAG *DAG, bool PreferReg) {
   assert(OpInfo.Codes.size() > 1 && "Doesn't have multiple constraint options");
   unsigned BestIdx = 0;
   TargetLowering::ConstraintType BestType = TargetLowering::C_Unknown;
   int BestGenerality = -1;
 
   // Loop over the options, keeping track of the most general one.
   for (unsigned i = 0, e = OpInfo.Codes.size(); i != e; ++i) {
     TargetLowering::ConstraintType CType =
for (unsigned i = 0, e = OpInfo.Codes.size(); i != e; ++i) {
     // Things with matching constraints can only be registers, per gcc
     // documentation.  This mainly affects "g" constraints.
     if (CType == TargetLowering::C_Memory && OpInfo.hasMatchingInput())
       continue;
 
     // This constraint letter is more general than the previous one, use it.
     int Generality = getConstraintGenerality(CType);
     if (Generality > BestGenerality) {
+      if (PreferReg && CType == TargetLowering::C_Memory &&
+          BestType == TargetLowering::C_RegisterClass)
+        continue;
       BestType = CType;
       BestIdx = i;
       BestGenerality = Generality;
     }
   }
 
   OpInfo.ConstraintCode = OpInfo.Codes[BestIdx];
   OpInfo.ConstraintType = BestType;
 }
 
 /// ComputeConstraintToUse - Determines the constraint code and constraint
 /// type to use for the specific AsmOperandInfo, setting
 /// OpInfo.ConstraintCode and OpInfo.ConstraintType.
-void TargetLowering::ComputeConstraintToUse(AsmOperandInfo &OpInfo,
-                                            SDValue Op,
-                                            SelectionDAG *DAG) const {
+void TargetLowering::ComputeConstraintToUse(AsmOperandInfo &OpInfo, SDValue Op,
+                                            SelectionDAG *DAG,
+                                            bool PreferReg) const {
   assert(!OpInfo.Codes.empty() && "Must have at least one constraint");
 
   // Single-letter constraints ('r') are very common.
   if (OpInfo.Codes.size() == 1) {
     OpInfo.ConstraintCode = OpInfo.Codes[0];
     OpInfo.ConstraintType = getConstraintType(OpInfo.ConstraintCode);
   } else {
-    ChooseConstraint(OpInfo, *this, Op, DAG);
+    ChooseConstraint(OpInfo, *this, Op, DAG, PreferReg);
   }
 
   // 'X' matches anything.
   if (OpInfo.ConstraintCode == "X" && OpInfo.CallOperandVal) {
     // Labels and constants are handled elsewhere ('X' is the only thing
     // that matches labels).  For Functions, the type here is the type of
     // the result, which is not what we want to look at; leave them alone.
     Value *v = OpInfo.CallOperandVal;

test/CodeGen/X86/inline-asm-rm-constraint.ll

+; RUN: llc -march=x86-64 < %s | FileCheck %s
+; Check that register is preferred vs memory when there is
+; only one input operand.
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
+target triple = "x86_64-apple-darwin10.0"
+
+declare i32 @bar(i32)
+
+define void @foo(i32* %codeLength, i32 %nextSeveralBits, i32 %order) {
+entry:
+; CHECK-LABEL:foo
+; CHECK: bsrl %esi, %ebp
+  %0 = tail call i32 asm "bsrl $1, $0", "=r,rm,~{dirflag},~{fpsr},~{flags}"(i32 %nextSeveralBits) #2
+  %call = tail call i32 @bar(i32 %nextSeveralBits)
+  %xor = shl i32 %0, 1
+  %mul = xor i32 %xor, 62
+  %add = add i32 %call, %order
+  %add1 = add i32 %add, %mul
+  store i32 %add1, i32* %codeLength, align 4
+  ret void
+}