IR: Fix use-list-order round-tripping for call and invoke

[dexonsmith]

Summary

Fix the use-list-order for call and invoke instructions by setting the
operands in order of their index. This matches the use-list-order
prediction. Note that the verifier precludes sharing arguments in
callbr, but I updated the code there as well for consistency.

Bug was found during review of https://reviews.llvm.org/D104740.

[nikic]

Can you please point me to the code where the use-list order prediction is made?

[dexonsmith]

Minor update to the testcase:

• Match the RUN: commands from uselistorder.ll.
• Rename to call-arg-is-callee.ll, since this is not testing the uselistorder directive... it's testing calls that have an argument as a callee.

[dexonsmith]

In D104805#2836738, @nikic wrote:

Can you please point me to the code where the use-list order prediction is made?

The relevant code for AsmWriter is:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/AsmWriter.cpp#L232

// LID and RID are equal, so we have different operands of the same user.
// Assume operands are added in order for all instructions.
if (GetsReversed)
  if (LID <= ID)
    return LU->getOperandNo() < RU->getOperandNo();
return LU->getOperandNo() > RU->getOperandNo();

I'll need a minute to find it for BitcodeWriter.

[dexonsmith]

In D104805#2836748, @dexonsmith wrote:

In D104805#2836738, @nikic wrote:

Can you please point me to the code where the use-list order prediction is made?

I'll need a minute to find it for BitcodeWriter.

Found it in ValueEnumerator.cpp:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Bitcode/Writer/ValueEnumerator.cpp#L249

// LID and RID are equal, so we have different operands of the same user.
// Assume operands are added in order for all instructions.
if (LID <= ID)
  if (!IsGlobalValue) // GlobalValue uses don't get reversed.
    return LU->getOperandNo() < RU->getOperandNo();
return LU->getOperandNo() > RU->getOperandNo();

Note that we could update the code in ValueEnumerator and AsmWriter instead, but IMO it's simpler to just assign the operands in order of index in the first place.

[nikic]

Thanks for the references! I agree that this is the better way to fix it, so LGTM.

llvm/lib/IR/Instructions.cpp

 //                        CallInst Implementation
 //===----------------------------------------------------------------------===//
 
 void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
                     ArrayRef<OperandBundleDef> Bundles, const Twine &NameStr) {
   this->FTy = FTy;
   assert(getNumOperands() == Args.size() + CountBundleInputs(Bundles) + 1 &&
          "NumOperands not set up?");
-  setCalledOperand(Func);
 
 #ifndef NDEBUG
   assert((Args.size() == FTy->getNumParams() ||
           (FTy->isVarArg() && Args.size() > FTy->getNumParams())) &&
          "Calling a function with bad signature!");
 
   for (unsigned i = 0; i != Args.size(); ++i)
     assert((i >= FTy->getNumParams() ||
             FTy->getParamType(i) == Args[i]->getType()) &&
            "Calling a function with a bad signature!");
 #endif
 
+  // Set operands in order of their index to match use-list-order
+  // prediction.
   llvm::copy(Args, op_begin());
+  setCalledOperand(Func);
 
   auto It = populateBundleOperandInfos(Bundles, Args.size());
   (void)It;
   assert(It + 1 == op_end() && "Should add up!");
 
   setName(NameStr);
 }
 
void InvokeInst::init(FunctionType *FTy, Value *Fn, BasicBlock *IfNormal,
                       BasicBlock *IfException, ArrayRef<Value *> Args,
                       ArrayRef<OperandBundleDef> Bundles,
                       const Twine &NameStr) {
   this->FTy = FTy;
 
   assert((int)getNumOperands() ==
              ComputeNumOperands(Args.size(), CountBundleInputs(Bundles)) &&
          "NumOperands not set up?");
-  setNormalDest(IfNormal);
-  setUnwindDest(IfException);
-  setCalledOperand(Fn);
 
 #ifndef NDEBUG
   assert(((Args.size() == FTy->getNumParams()) ||
           (FTy->isVarArg() && Args.size() > FTy->getNumParams())) &&
          "Invoking a function with bad signature");
 
   for (unsigned i = 0, e = Args.size(); i != e; i++)
     assert((i >= FTy->getNumParams() ||
             FTy->getParamType(i) == Args[i]->getType()) &&
            "Invoking a function with a bad signature!");
 #endif
 
+  // Set operands in order of their index to match use-list-order
+  // prediction.
   llvm::copy(Args, op_begin());
+  setNormalDest(IfNormal);
+  setUnwindDest(IfException);
+  setCalledOperand(Fn);
 
   auto It = populateBundleOperandInfos(Bundles, Args.size());
   (void)It;
   assert(It + 3 == op_end() && "Should add up!");
 
   setName(NameStr);
 }
 
void CallBrInst::init(FunctionType *FTy, Value *Fn, BasicBlock *Fallthrough,
                       ArrayRef<OperandBundleDef> Bundles,
                       const Twine &NameStr) {
   this->FTy = FTy;
 
   assert((int)getNumOperands() ==
              ComputeNumOperands(Args.size(), IndirectDests.size(),
                                 CountBundleInputs(Bundles)) &&
          "NumOperands not set up?");
-  NumIndirectDests = IndirectDests.size();
-  setDefaultDest(Fallthrough);
-  for (unsigned i = 0; i != NumIndirectDests; ++i)
-    setIndirectDest(i, IndirectDests[i]);
-  setCalledOperand(Fn);
 
 #ifndef NDEBUG
   assert(((Args.size() == FTy->getNumParams()) ||
           (FTy->isVarArg() && Args.size() > FTy->getNumParams())) &&
          "Calling a function with bad signature");
 
   for (unsigned i = 0, e = Args.size(); i != e; i++)
     assert((i >= FTy->getNumParams() ||
             FTy->getParamType(i) == Args[i]->getType()) &&
            "Calling a function with a bad signature!");
 #endif
 
+  // Set operands in order of their index to match use-list-order
+  // prediction.
   std::copy(Args.begin(), Args.end(), op_begin());
+  NumIndirectDests = IndirectDests.size();
+  setDefaultDest(Fallthrough);
+  for (unsigned i = 0; i != NumIndirectDests; ++i)
+    setIndirectDest(i, IndirectDests[i]);
+  setCalledOperand(Fn);
 
   auto It = populateBundleOperandInfos(Bundles, Args.size());
   (void)It;
   assert(It + 2 + IndirectDests.size() == op_end() && "Should add up!");
 
   setName(NameStr);
 }
 

llvm/test/Assembler/call-arg-is-callee.ll

+; RUN: llvm-as < %s -disable-output 2>&1 | FileCheck %s -allow-empty
+; CHECK-NOT: error
+; CHECK-NOT: warning
+; RUN: verify-uselistorder < %s
+
+; Check ordering of callee operand versus the argument operand.
+define void @call(void (...)* %p) {
+  call void (...) %p(void (...)* %p)
+  ret void
+}
+
+; Check ordering of callee operand versus the argument operand.
+declare void @personality(i8*)
+define void @invoke(void (...)* %p) personality void(i8*)* @personality {
+entry:
+  invoke void (...) %p(void (...)* %p)
+  to label %normal unwind label %exception
+normal:
+  ret void
+exception:
+  landingpad { i8*, i32 } cleanup
+  ret void
+}
+
+; Check order for callbr instruction. Cannot reuse labels in the test since the
+; verifier prevents duplicating callbr destinations.
+define void @callbr() {
+entry:
+  callbr i32 asm "", "=r,r,X,X"(i32 0,
+                                i8 *blockaddress(@callbr, %two),
+                                i8 *blockaddress(@callbr, %three))
+              to label %one [label %two, label %three]
+one:
+  ret void
+two:
+  ret void
+three:
+  ret void
+}