[llvm-reduce] Reduce function calling convention

[aeubanks]

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

46

I think this should be a list of CCs to opt-out, rather than opt-in to calling convention reduction since it really should be fine to reduce them to CCC in terms of legality.

For the amdgpu entry-point concerns, that's the point of the interestingness test?

[aeubanks]

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

46

oh I forgot to ask the initial question, if you do want to opt out some CCs, which ones do you want to opt out?

[arsenm]

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

46

I think opt-in is basically always a better system, especially for target stuff (which is what any unlisted CC is going to be)

[aeubanks]

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

46

llvm-reduce is designed to reduce IR as much as possible and I don't think this is an exception, like I said the interestingness test should take care of if you really don't want the CC reduced

[aeubanks]

(adding more people for more opinions)

[arsenm]

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

46

You're asserting that cc is reduced or simpler and other calling conventions are not, and I do not think you can view them this way. From an IR perspective it's not really conceptually simpler, it just removes one token from the printed IR. With the possible exception of fastcc, it's not really an optimizing modifier

What it does do is change codegen in arbitrary ways, and I don't think that provides any reduction value on the IR side.

[dblaikie]

FWIW, I'd consider this a simplification - if I saw a test case that used a novel/explicit calling convention, I'd probably assume/guess that the CC was load bearing/required/interesting/relevant to the bug. So a test case that doesn't use a novel CC is "simpler" in that sense, to me at least. (it's not a guarantee, it's possible the bug is only reproducible with the default CC, but more often than not that's not going to be the case, I think?)

[compnerd]

In D155638#4515322, @dblaikie wrote:

FWIW, I'd consider this a simplification - if I saw a test case that used a novel/explicit calling convention, I'd probably assume/guess that the CC was load bearing/required/interesting/relevant to the bug. So a test case that doesn't use a novel CC is "simpler" in that sense, to me at least. (it's not a guarantee, it's possible the bug is only reproducible with the default CC, but more often than not that's not going to be the case, I think?)

I agree. There are some special cases for some CCs in the codebase, so this definitely helps identify that behavioural difference.

[arsenm]

In D155638#4515322, @dblaikie wrote:

FWIW, I'd consider this a simplification - if I saw a test case that used a novel/explicit calling convention, I'd probably assume/guess that the CC was load bearing/required/interesting/relevant to the bug. So a test case that doesn't use a novel CC is "simpler" in that sense, to me at least. (it's not a guarantee, it's possible the bug is only reproducible with the default CC, but more often than not that's not going to be the case, I think?)

This is a very x86 centric view. I might go so far as to consider CCC to be the most complex of the calling conventions, others add restrictions

[nikic]

In D155638#4515356, @arsenm wrote:

In D155638#4515322, @dblaikie wrote:

FWIW, I'd consider this a simplification - if I saw a test case that used a novel/explicit calling convention, I'd probably assume/guess that the CC was load bearing/required/interesting/relevant to the bug. So a test case that doesn't use a novel CC is "simpler" in that sense, to me at least. (it's not a guarantee, it's possible the bug is only reproducible with the default CC, but more often than not that's not going to be the case, I think?)

This is a very x86 centric view. I might go so far as to consider CCC to be the most complex of the calling conventions, others add restrictions

It's an IR centric view. Calling convention is generally irrelevant for middle-end tests and should be omitted from them.

For codegen tests, if the calling convention is relevant to reproduce an issue, that should be determined by the interestingness test.

[arsenm]

In D155638#4515831, @nikic wrote:

It's an IR centric view. Calling convention is generally irrelevant for middle-end tests and should be omitted from them.

This isn't a middle end test, it's a general IR tool

For codegen tests, if the calling convention is relevant to reproduce an issue, that should be determined by the interestingness test.

Requiring additional complexity in interestingness tests is not good. The ideal interestingness test is just run the crashing program, I shouldn't have to hack around odd tooling decisions. The calling convention can and does place IR constraints which this is not considering, and we should try to avoid failing the verifier in any reduction.

Basically I'm fine with reducing fastcc, coldcc, and tailcc to c and opposed to "reducing" any other arbitrary CC. I don't see arbitrarily changing the CC as opening new reduction opportunities, and is easy to manually remove as the final step

[nikic]

In D155638#4516642, @arsenm wrote:

In D155638#4515831, @nikic wrote:

It's an IR centric view. Calling convention is generally irrelevant for middle-end tests and should be omitted from them.

This isn't a middle end test, it's a general IR tool

Sure, but it works on IR, so it should do an IR focused reduction.

I think we have a lot of precedent for this. For example, we reduce away the "dso_local" flag, which is a simplification on the IR level, but likely makes codegen more complex. Similarly, we reduce globals to external linkage -- again, this likely makes codegen more complex. Etc.

llvm-reduce at least right now favors producing minimal IR, not producing IR that gives you the minimum number of asm instructions after codegen.

If we wanted llvm-reduce to optimize for codegen tests, it should probably do things like add nounwind attributes to functions to avoid CFI, convert globals to internal linkage to avoid relocations, etc.

Requiring additional complexity in interestingness tests is not good. The ideal interestingness test is just run the crashing program, I shouldn't have to hack around odd tooling decisions.

You don't need to explicitly add something to the interestingness test. If the crash reproduces only with a specific CC, then you'll get that CC. If it doesn't require a specific CC, then it's okay to remove it.

The calling convention can and does place IR constraints which this is not considering, and we should try to avoid failing the verifier in any reduction.

I do agree with this part -- do you have any example where removing the calling convention would cause a verifier error? I would have assumed that the default cc doesn't have any special restrictions.

[aeubanks]

If we wanted llvm-reduce to optimize for codegen tests, it should probably do things like add nounwind attributes to functions to avoid CFI, convert globals to internal linkage to avoid relocations, etc.

In the past I've suggested a llvm-reduce flag that would change the reduction behavior to do this since I can definitely see it being useful for debugging codegen issues, but llvm-reduce is a general IR reduction tool which I mostly view as providing nice minimal test cases (text-wise) to put into llvm tests.

[arsenm]

In D155638#4517796, @nikic wrote:

I think we have a lot of precedent for this. For example, we reduce away the "dso_local" flag, which is a simplification on the IR level, but likely makes codegen more complex. Similarly, we reduce globals to external linkage -- again, this likely makes codegen more complex. Etc.

This isn't a great analog. dso_local at least fits the mold of an optimization flag. It's not something mandatory. A target should have the right to not implement CCC at all and only use custom calling conventions (this was the case for AMDGPU at the very beginning for example).

llvm-reduce at least right now favors producing minimal IR, not producing IR that gives you the minimum number of asm instructions after codegen.

If we wanted llvm-reduce to optimize for codegen tests, it should probably do things like add nounwind attributes to functions to avoid CFI, convert globals to internal linkage to avoid relocations, etc.

I've thought about doing this, but it is a different thing

[nikic]

In D155638#4520748, @arsenm wrote:

In D155638#4517796, @nikic wrote:

I think we have a lot of precedent for this. For example, we reduce away the "dso_local" flag, which is a simplification on the IR level, but likely makes codegen more complex. Similarly, we reduce globals to external linkage -- again, this likely makes codegen more complex. Etc.

This isn't a great analog. dso_local at least fits the mold of an optimization flag. It's not something mandatory. A target should have the right to not implement CCC at all and only use custom calling conventions (this was the case for AMDGPU at the very beginning for example).

So the concern here is that if a target doesn't implement it, then it's likely that we'll get a false positive interesting test when run against something like ! llc? I think for that particular case (CCC not supported) it would be fine to opt-out the relevant calling convention.

[arsenm]

In D155638#4531620, @nikic wrote:

So the concern here is that if a target doesn't implement it, then it's likely that we'll get a false positive interesting test when run against something like ! llc? I think for that particular case (CCC not supported) it would be fine to opt-out the relevant calling convention.

Opt out target stuff has been a constant source of pain in llvm. Opt in is a much more manageable system. I don’t see a practical plus to handling anything other than the handful of common CCs (fast, cold, tail)

[dblaikie]

So the concern here is that if a target doesn't implement it, then it's likely that we'll get a false positive interesting test when run against something like ! llc?

Personally, I don't think this is a use case worth optimizing for. A match that open ended is bound to trip over other issues - reduce to the wrong crasher, etc. Anything vaguely precise enough to be useful probably isn't going to get tripped up by an attempt to use a calling convention incompatible with the target, I'd have thought?

What's the failure look like when you use a cc incompatible with the target? Does it fail in IR parsing?

llvm/test/tools/llvm-reduce/reduce-calling-convention.ll

+; Test that llvm-reduce can remove function calling conventions.
+;
+; RUN: llvm-reduce --abort-on-invalid-reduction --delta-passes=function-data --test FileCheck --test-arg --check-prefixes=CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: FileCheck --check-prefix=CHECK-FINAL --implicit-check-not=fastcc %s < %t
+
+; CHECK-INTERESTINGNESS: declare
+; CHECK-INTERESTINGNESS-SAME: void @f
+
+; CHECK-FINAL: declare void @f()
+
+declare void @f()
+
+; CHECK-INTERESTINGNESS: declare
+; CHECK-INTERESTINGNESS-SAME: void @amdgpu
+
+; CHECK-FINAL: declare amdgpu_cs_chain void @amdgpu()
+
+declare amdgpu_cs_chain void @amdgpu()
+
+; CHECK-INTERESTINGNESS: declare
+; CHECK-INTERESTINGNESS-SAME: void @g
+
+; CHECK-FINAL: declare void @g()
+
+declare fastcc void @g()
+
+; CHECK-FINAL: define void @callg()
+; CHECK-FINAL: call void @g()
+define void @callg() {
+  call fastcc void @g()
+  ret void
+}

llvm/tools/llvm-reduce/deltas/ReduceFunctionBodies.cpp

 //===- ReduceFunctions.cpp - Specialized Delta Pass -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a function which calls the Generic Delta pass in order
 // to reduce function bodies in the provided Module.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ReduceFunctionBodies.h"
 #include "Delta.h"
 #include "Utils.h"
+#include "llvm/IR/CallingConv.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Instructions.h"
 
 using namespace llvm;
 
 /// Removes all the bodies of defined functions that aren't inside any of the
 /// desired Chunks.
 static void extractFunctionBodiesFromModule(Oracle &O,
                                             ReducerWorkItem &WorkItem) {
   // Delete out-of-chunk function bodies
   for (auto &F : WorkItem.getModule()) {
     if (!F.isDeclaration() && !hasAliasUse(F) && !O.shouldKeep()) {
       F.deleteBody();
       F.setComdat(nullptr);
     }
   }
 }
 
 void llvm::reduceFunctionBodiesDeltaPass(TestRunner &Test) {
   runDeltaPass(Test, extractFunctionBodiesFromModule,
                "Reducing Function Bodies");
 }
 
+static bool shouldReduceCallingConvention(Function &F) {
+  // The CCC is very lenient, so most calling conventions should be able to be
+  // changed to it.
+  switch (F.getCallingConv()) {
+  case CallingConv::C:
+  case CallingConv::AMDGPU_CS_Chain:
+    return false;
+  default:
+    return true;
+  }
+}
+
 static void reduceFunctionData(Oracle &O, ReducerWorkItem &WorkItem) {
   for (Function &F : WorkItem.getModule()) {
     if (F.hasPersonalityFn()) {
       if (none_of(F,
                   [](const BasicBlock &BB) {
                     return BB.isEHPad() || isa<ResumeInst>(BB.getTerminator());
                   }) &&
           !O.shouldKeep()) {
         F.setPersonalityFn(nullptr);
       }
     }
 
     if (F.hasPrefixData() && !O.shouldKeep())
       F.setPrefixData(nullptr);
 
     if (F.hasPrologueData() && !O.shouldKeep())
       F.setPrologueData(nullptr);
+
+    if (shouldReduceCallingConvention(F) && !O.shouldKeep()) {
+      // Update callee calling conventions to avoid UB when possible.
+      for (Use &U : F.uses()) {
+        if (auto *CB = dyn_cast<CallBase>(U.getUser())) {
+          if (&CB->getCalledOperandUse() == &U)
+            CB->setCallingConv(CallingConv::C);
+        }
+      }
+      F.setCallingConv(CallingConv::C);
+    }
   }
 }
 
 void llvm::reduceFunctionDataDeltaPass(TestRunner &Test) {
   runDeltaPass(Test, reduceFunctionData, "Reducing Function Data");
 }