Use function attribute "trap-func-name" and remove TargetOptions::TrapFuncName

[ahatanak]

Summary

This patch changes normal isel and fast-isel to read the user-defined trap function name from function attribute "trap-func-name" instead of from TargetOptions::TrapFuncName.

This is the llvm side change that goes with this clang patch:
http://reviews.llvm.org/D10831

[echristo]

Couple of small inline comments.

Thanks!

-eric

lib/CodeGen/SelectionDAG/FastISel.cpp

1351 ↗

(On Diff #28754)

Fix the typo in the comment while you're here? ;)

1353 ↗

(On Diff #28754)

MF->getFunction() :)

[ahatanak]

Thanks.

I realized while reading the review comments that I could attach the attribute to the CallInst instead of the caller function. That way, we can don't have to check that two functions have the same "trap-func-name" value when the functions are inlined or merged.

call void @llvm.trap() #0
...
attributes #0 = { noreturn nounwind "trap-func-name"="foo111" }

Do you see a problem with this approach?

[echristo]

I think that'll cause problems when we merge translation units at module
link time? i.e. we only have one llvm.trap intrinsic.

-eric

[ahatanak]

In D10832#197327, @echristo wrote:

I think that'll cause problems when we merge translation units at module
link time? i.e. we only have one llvm.trap intrinsic.

Attaching the attribute to the declaration of llvm.trap would cause problems when linking modules, but attaching it to the call site should be fine. The linked module looks like this:

declare void @llvm.trap() #2
...
call void @llvm.trap() #4
...
call void @llvm.trap() #5
...
attributes #2 = { noreturn nounwind }
attributes #4 = { noreturn nounwind "trap-func-name"="foo112" }
attributes #5 = { noreturn nounwind "trap-func-name"="foo111" }

[echristo]

How would you deal with inlining?

-eric

[ahatanak]

In the following example, we can just inline test0 into test1 without checking "trap-func-name" on the call-sites, no? If we attach the attribute to the calling functions, then we'll have to check that the merged functions have the same trap function name.

declare void @llvm.trap() #2
...
define void @test1() #0 {

...
call void @llvm.trap() #4
...
call void @test0()

}

define void @test0() #3 {

...
call void @llvm.trap() #5
...

}

attributes #2 = { noreturn nounwind }
attributes #4 = { noreturn nounwind "trap-func-name"="foo112" }
attributes #5 = { noreturn nounwind "trap-func-name"="foo111" }

[echristo]

Right. I mean, neither of them will "win" which may be what we wanted. Or
not. I don't know. :)

(LTO code generation options ftw? :)

Either way, I don't see it as too much of an issue if you want to go this
way instead.

-eric

[ahatanak]

Here is my new patch. Sorry for asking for a review again.

• Changed normal isel and fast isel to read the "trap-func-name" attached to the call-site.

• Added new functions that are needed to access and modify string attributes attached to CallInst.

• Made changes to setFunctionAttributes that are needed to override "trap-func-name" in the IR via command line option "trap-func". If the command line option is used, iterate over all instructions in the function and rewrite the function attribute attached to the call sites. Alternatively, I think we can iterate over the list of AttributeSetImpl owned by LLVMContextImpl if visiting all instructions is too expensive.

[echristo]

Seems reasonable to me. One small inline request that you can do or not do as you prefer :)

-eric

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4783–4784 ↗

(On Diff #28834)

I realize auto is cool and all, but it seems like a bit of overkill here :)

[ahatanak]

Replaced auto with explicit type and removed the line which was causing unused variable warning. Also, changed a line in the test case to check for "trap" but not "trap_llc" or "trap_func_attr".

I'll commit this shortly. Thank you for the review!

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

 // flag duplication.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_COMMANDFLAGS_H
 #define LLVM_CODEGEN_COMMANDFLAGS_H
 
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/MC/MCTargetOptionsCommandFlags.h"
 #include "llvm//MC/SubtargetFeature.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
static inline TargetOptions InitTargetOptionsFromCodeGenFlags() {
   Options.NoNaNsFPMath = EnableNoNaNsFPMath;
   Options.HonorSignDependentRoundingFPMathOption =
       EnableHonorSignDependentRoundingFPMath;
   if (FloatABIForCalls != FloatABI::Default)
     Options.FloatABIType = FloatABIForCalls;
   Options.NoZerosInBSS = DontPlaceZerosInBSS;
   Options.GuaranteedTailCallOpt = EnableGuaranteedTailCallOpt;
   Options.StackAlignmentOverride = OverrideStackAlignment;
-  Options.TrapFuncName = TrapFuncName;
   Options.PositionIndependentExecutable = EnablePIE;
   Options.UseInitArray = !UseCtors;
   Options.DataSections = DataSections;
   Options.FunctionSections = FunctionSections;
   Options.UniqueSectionNames = UniqueSectionNames;
 
   Options.MCOptions = InitMCTargetOptionsFromFlags();
   Options.JTType = JTableType;
if (DisableFPElim.getNumOccurrences() > 0)
                                        "no-frame-pointer-elim",
                                        DisableFPElim ? "true" : "false");
 
     if (DisableTailCalls.getNumOccurrences() > 0)
       NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
                                        "disable-tail-calls",
                                        toStringRef(DisableTailCalls));
 
+    if (TrapFuncName.getNumOccurrences() > 0)
+      for (auto &B : F)
+        for (auto &I : B)
+          if (auto *Call = dyn_cast<CallInst>(&I))
+            if (const auto *F = Call->getCalledFunction())
+              if (F->getIntrinsicID() == Intrinsic::debugtrap ||
+                  F->getIntrinsicID() == Intrinsic::trap)
+                Call->addAttribute(llvm::AttributeSet::FunctionIndex,
+                                   "trap-func-name", TrapFuncName);
+
     // Let NewAttrs override Attrs.
     NewAttrs = Attrs.addAttributes(Ctx, AttributeSet::FunctionIndex, NewAttrs);
     F.setAttributes(NewAttrs);
   }
 }
 
 #endif

llvm/trunk/include/llvm/Target/TargetOptions.h

TargetOptions()
           HonorSignDependentRoundingFPMathOption(false),
           NoZerosInBSS(false),
           GuaranteedTailCallOpt(false),
           StackAlignmentOverride(0),
           EnableFastISel(false), PositionIndependentExecutable(false),
           UseInitArray(false), DisableIntegratedAS(false),
           CompressDebugSections(false), FunctionSections(false),
           DataSections(false), UniqueSectionNames(true), TrapUnreachable(false),
-          TrapFuncName(), FloatABIType(FloatABI::Default),
+          FloatABIType(FloatABI::Default),
           AllowFPOpFusion(FPOpFusion::Standard), Reciprocals(TargetRecip()),
           JTType(JumpTable::Single),
           ThreadModel(ThreadModel::POSIX) {}
 
     /// PrintMachineCode - This flag is enabled when the -print-machineinstrs
     /// option is specified on the command line, and should enable debugging
     /// output from the code generator.
     unsigned PrintMachineCode : 1;
public:
     /// Emit data into separate sections.
     unsigned DataSections : 1;
 
     unsigned UniqueSectionNames : 1;
 
     /// Emit target-specific trap instruction for 'unreachable' IR instructions.
     unsigned TrapUnreachable : 1;
 
-    /// getTrapFunctionName - If this returns a non-empty string, this means
-    /// isel should lower Intrinsic::trap to a call to the specified function
-    /// name instead of an ISD::TRAP node.
-    std::string TrapFuncName;
-    StringRef getTrapFunctionName() const;
-
     /// FloatABIType - This setting is set by -float-abi=xxx option is specfied
     /// on the command line. This setting may either be Default, Soft, or Hard.
     /// Default selects the target's default behavior. Soft selects the ABI for
     /// software floating point, but does not indicate that FP hardware may not
     /// be used. Such a combination is unfortunately popular (e.g.
     /// arm-apple-darwin). Hard presumes that the normal FP ABI is used.
     FloatABI::ABIType FloatABIType;
 
return
     ARE_EQUAL(HonorSignDependentRoundingFPMathOption) &&
     ARE_EQUAL(NoZerosInBSS) &&
     ARE_EQUAL(GuaranteedTailCallOpt) &&
     ARE_EQUAL(StackAlignmentOverride) &&
     ARE_EQUAL(EnableFastISel) &&
     ARE_EQUAL(PositionIndependentExecutable) &&
     ARE_EQUAL(UseInitArray) &&
     ARE_EQUAL(TrapUnreachable) &&
-    ARE_EQUAL(TrapFuncName) &&
     ARE_EQUAL(FloatABIType) &&
     ARE_EQUAL(AllowFPOpFusion) &&
     ARE_EQUAL(Reciprocals) &&
     ARE_EQUAL(JTType) &&
     ARE_EQUAL(ThreadModel) &&
     ARE_EQUAL(MCOptions);
 #undef ARE_EQUAL
 }

llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp

if (const auto *Call = dyn_cast<CallInst>(I)) {
     // may be translated directly to target instructions.
     if (F && !F->hasLocalLinkage() && F->hasName() &&
         LibInfo->getLibFunc(F->getName(), Func) &&
         LibInfo->hasOptimizedCodeGen(Func))
       return false;
 
     // Don't handle Intrinsic::trap if a trap funciton is specified.
     if (F && F->getIntrinsicID() == Intrinsic::trap &&
-        !TM.Options.getTrapFunctionName().empty())
+        Call->hasFnAttr("trap-func-name"))
       return false;
   }
 
   // First, try doing target-independent selection.
   if (!SkipTargetIndependentISel) {
     if (selectOperator(I, I->getOpcode())) {
       ++NumFastIselSuccessIndependent;
       DbgLoc = DebugLoc();

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, unsigned Intrinsic) {
   case Intrinsic::expect: {
     // Just replace __builtin_expect(exp, c) with EXP.
     setValue(&I, getValue(I.getArgOperand(0)));
     return nullptr;
   }
 
   case Intrinsic::debugtrap:
   case Intrinsic::trap: {
-    StringRef TrapFuncName = TM.Options.getTrapFunctionName();
+    StringRef TrapFuncName =
+        I.getAttributes()
+            .getAttribute(AttributeSet::FunctionIndex, "trap-func-name")
+            .getValueAsString();
     if (TrapFuncName.empty()) {
       ISD::NodeType Op = (Intrinsic == Intrinsic::trap) ?
         ISD::TRAP : ISD::DEBUGTRAP;
       DAG.setRoot(DAG.getNode(Op, sdl,MVT::Other, getRoot()));
       return nullptr;
     }
     TargetLowering::ArgListTy Args;
 

llvm/trunk/lib/CodeGen/TargetOptionsImpl.cpp

bool TargetOptions::LessPreciseFPMAD() const {
   return UnsafeFPMath || LessPreciseFPMADOption;
 }
 
 /// HonorSignDependentRoundingFPMath - Return true if the codegen must assume
 /// that the rounding mode of the FPU can change from its default.
 bool TargetOptions::HonorSignDependentRoundingFPMath() const {
   return !UnsafeFPMath && HonorSignDependentRoundingFPMathOption;
 }
-
-/// getTrapFunctionName - If this returns a non-empty string, this means isel
-/// should lower Intrinsic::trap to a call to the specified function name
-/// instead of an ISD::TRAP node.
-StringRef TargetOptions::getTrapFunctionName() const {
-  return TrapFuncName;
-}

llvm/trunk/test/CodeGen/ARM/fnattr-trap.ll

+; RUN: llc < %s -march arm | FileCheck %s -check-prefix=NOOPTION
+; RUN: llc < %s -march arm -trap-func=trap_llc | FileCheck %s -check-prefix=TRAP
+
+; NOOPTION-LABEL: {{\_?}}foo0:
+; NOOPTION: trap{{$}}
+
+; TRAP-LABEL: {{\_?}}foo0:
+; TRAP: bl {{\_?}}trap_llc
+
+define void @foo0() {
+  call void @llvm.trap()
+  unreachable
+}
+
+; NOOPTION-LABEL: {{\_?}}foo1:
+; NOOPTION: bl {{\_?}}trap_func_attr0
+
+; TRAP-LABEL: {{\_?}}foo1:
+; TRAP: bl {{\_?}}trap_llc
+
+define void @foo1() {
+  call void @llvm.trap() #0
+  unreachable
+}
+
+; NOOPTION-LABEL: {{\_?}}foo2:
+; NOOPTION: bl {{\_?}}trap_func_attr1
+
+; TRAP-LABEL: {{\_?}}foo2:
+; TRAP: bl {{\_?}}trap_llc
+
+define void @foo2() {
+  call void @llvm.trap() #1
+  unreachable
+}
+
+declare void @llvm.trap() nounwind
+
+attributes #0 = { "trap-func-name"="trap_func_attr0" }
+attributes #1 = { "trap-func-name"="trap_func_attr1" }