[PATCH] Adding a new [[clang::impossible_enum]] attribute to clang
Aaron Ballman
aaron.ballman at gmail.com
Fri Jan 2 14:47:33 PST 2015
On Mon, Dec 22, 2014 at 6:34 PM, Chris Bieneman <beanz at apple.com> wrote:
> Hi alexr, aaron.ballman,
>
> This attribute provides a way to instruct the compiler that a given enum value is never valid. This specification can allow large segments of code to be dead stripped.
>
> One hypothetical use of this is my RFC on stripping intrinsics for un-used targets in LLVM (http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079818.html).
>
> In this case the attribute would allow us to remove the code and constant data for unusable attributes without needing to litter around #ifdefs.
>
> Disclaimer: This idea came from Alex Rosenberg... Beware.
I agree with Reid's suggestion to hold off for now, but in case you do
decide to move forward, some comments below:
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -1996,3 +1996,10 @@
> let SemaHandler = 0;
> let Documentation = [Undocumented];
> }
> +
> +def ImpossibleEnum : Attr {
> + let Spellings = [CXX11<"clang","impossible_enum">];
Missing a space after the comma. Also, I'm not keen on the name --
it's not an impossible enum, it's an enumerator that results in
dead-stripping code.
> + let Subjects = SubjectList<[EnumConstant], ErrorDiag>;
I'm not convinced this one is worth a dedicated subject. I think a
SubsetSubject would make more sense for right now.
> + let Documentation = [ImpossibleEnumDocs];
> +}
> +
> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -1421,3 +1421,17 @@
> cannot point to the private address space.
> }];
> }
> +
> +def ImpossibleEnumDocs : Documentation {
> + let Category = DocCatVariable;
> + let Heading = "[[clang::impossible_enum]]";
> + let Content = [{
> +The impossible_enum attribute specifies to the compiler that enum values with
> +this attribute are never valid. This allows the compiler to treat branches based
> +on this enum value as dead code. This attribute only impacts the behavior of
> +switch statements and conditional expressions. In switch statements any case
> +labels which are based on an impossible_enum are not emitted to the IR. In
> +conditional expressions equality against impossible_enum values is always false,
> +all other boolean operators are true.
Given:
enum E {
good = 1 << 0,
mediocre = 1 << 1,
bad [[clang::impossible_enum]] = 1 << 2
};
Would this code emit different IR than if the attribute was elided?
void f(E e) {
if (e & bad == bad) {}
}
What about?
E e = bad;
if (e == 1 << 2 /* stupid idea */ ) {}
> + }];
> +}
> Index: include/clang/Sema/AttributeList.h
> ===================================================================
> --- include/clang/Sema/AttributeList.h
> +++ include/clang/Sema/AttributeList.h
> @@ -823,6 +823,7 @@
> ExpectedFunctionMethodOrParameter,
> ExpectedClass,
> ExpectedEnum,
> + ExpectedEnumConstant,
> ExpectedVariable,
> ExpectedMethod,
> ExpectedVariableFunctionOrLabel,
> Index: lib/CodeGen/CGExprScalar.cpp
> ===================================================================
> --- lib/CodeGen/CGExprScalar.cpp
> +++ lib/CodeGen/CGExprScalar.cpp
> @@ -2770,9 +2770,23 @@
> Value *ScalarExprEmitter::EmitCompare(const BinaryOperator *E,unsigned UICmpOpc,
> unsigned SICmpOpc, unsigned FCmpOpc) {
> TestAndClearIgnoreResultAssign();
> - Value *Result;
> + Value *Result = nullptr;
> QualType LHSTy = E->getLHS()->getType();
> QualType RHSTy = E->getRHS()->getType();
> +
> + auto *LHSDeclRef = dyn_cast<DeclRefExpr>(E->getLHS()->IgnoreCasts());
> + auto *RHSDeclRef = dyn_cast<DeclRefExpr>(E->getRHS()->IgnoreCasts());
> + bool impossibleEnum =
> + (LHSDeclRef && LHSDeclRef->getDecl()->hasAttr<ImpossibleEnumAttr>()) ||
> + (RHSDeclRef && RHSDeclRef->getDecl()->hasAttr<ImpossibleEnumAttr>());
> + if (impossibleEnum) {
> + if (E->getOpcode() == BO_EQ)
> + Result = llvm::ConstantInt::getFalse(ConvertType(E->getType()));
> + else
> + Result = llvm::ConstantInt::getTrue(ConvertType(E->getType()));
So >, <, <=, and >= would all result in a true result?
> + return EmitScalarConversion(Result, CGF.getContext().BoolTy, E->getType());
> + }
> +
> if (const MemberPointerType *MPT = LHSTy->getAs<MemberPointerType>()) {
> assert(E->getOpcode() == BO_EQ ||
> E->getOpcode() == BO_NE);
> Index: lib/CodeGen/CGStmt.cpp
> ===================================================================
> --- lib/CodeGen/CGStmt.cpp
> +++ lib/CodeGen/CGStmt.cpp
> @@ -13,6 +13,7 @@
>
> #include "CodeGenFunction.h"
> #include "CGDebugInfo.h"
> +#include "CGBuilder.h"
> #include "CodeGenModule.h"
> #include "TargetInfo.h"
> #include "clang/AST/StmtVisitor.h"
> @@ -1252,11 +1253,21 @@
> }
> }
>
> - llvm::BasicBlock *CaseDest = createBasicBlock("sw.bb");
> - EmitBlockWithFallThrough(CaseDest, CaseCnt);
> + const DeclRefExpr *EnumDeclExpr =
> + dyn_cast<DeclRefExpr>(S.getLHS()->IgnoreCasts());
> + bool isCaseImpossible =
> + EnumDeclExpr && EnumDeclExpr->getDecl()->hasAttr<ImpossibleEnumAttr>();
> +
> + llvm::BasicBlock *CaseDest = nullptr;
> if (SwitchWeights)
> SwitchWeights->push_back(CaseCnt.getCount());
> - SwitchInsn->addCase(CaseVal, CaseDest);
> + if (!isCaseImpossible) {
> + if (!CaseDest) {
> + CaseDest = createBasicBlock("sw.bb");
> + EmitBlockWithFallThrough(CaseDest, CaseCnt);
> + }
> + SwitchInsn->addCase(CaseVal, CaseDest);
> + }
>
> // Recursively emitting the statement is acceptable, but is not wonderful for
> // code where we have many case statements nested together, i.e.:
> @@ -1276,15 +1287,25 @@
> llvm::ConstantInt *CaseVal =
> Builder.getInt(CurCase->getLHS()->EvaluateKnownConstInt(getContext()));
>
> + EnumDeclExpr = dyn_cast<DeclRefExpr>(CurCase->getLHS()->IgnoreCasts());
> + isCaseImpossible =
> + EnumDeclExpr && EnumDeclExpr->getDecl()->hasAttr<ImpossibleEnumAttr>();
> +
> CaseCnt = getPGORegionCounter(NextCase);
> if (SwitchWeights)
> SwitchWeights->push_back(CaseCnt.getCount());
> if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
> CaseDest = createBasicBlock("sw.bb");
> EmitBlockWithFallThrough(CaseDest, CaseCnt);
> }
>
> - SwitchInsn->addCase(CaseVal, CaseDest);
> + if (!isCaseImpossible) {
> + if (!CaseDest) {
> + CaseDest = createBasicBlock("sw.bb");
> + EmitBlockWithFallThrough(CaseDest, CaseCnt);
> + }
> + SwitchInsn->addCase(CaseVal, CaseDest);
> + }
> NextCase = dyn_cast<CaseStmt>(CurCase->getSubStmt());
> }
>
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -3201,6 +3201,16 @@
> Attr.getAttributeSpellingListIndex()));
> }
>
> +static void handleImpossibleEnumAttr(Sema &S, Decl *D,
> + const AttributeList &Attr) {
> + if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> + return;
Should document this restriction.
> +
> + D->addAttr(::new (S.Context)
> + ImpossibleEnumAttr(Attr.getRange(), S.Context,
> + Attr.getAttributeSpellingListIndex()));
Formatting appears to be off by a bit.
> +}
> +
> static void handleMinSizeAttr(Sema &S, Decl *D,
> const AttributeList &Attr) {
> if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> @@ -4367,6 +4377,9 @@
> case AttributeList::AT_AlwaysInline:
> handleAlwaysInlineAttr(S, D, Attr);
> break;
> + case AttributeList::AT_ImpossibleEnum:
> + handleImpossibleEnumAttr(S, D, Attr);
> + break;
> case AttributeList::AT_AnalyzerNoReturn:
> handleAnalyzerNoReturnAttr(S, D, Attr);
> break;
> Index: test/CodeGenCXX/impossible_enum.cpp
> ===================================================================
> --- /dev/null
> +++ test/CodeGenCXX/impossible_enum.cpp
> @@ -0,0 +1,88 @@
> +// RUN: %clang_cc1 -std=c++11 -emit-llvm %s -o - | FileCheck %s
> +
> +void printf(const char*, ...);
> +
> +enum ImpossibleEnum {
> + value1,
> + value2,
> + value3 [[clang::impossible_enum]]
> +};
> +
> +// CHECK: switch
> +// CHECK: label
> +// CHECK: label
> +// CHECK: ]
> +// CHECK: call
> +// CHECK: call
> +// CHECK: ret void
> +void foo(int i) {
> + switch(i) {
> + case value1:
> + printf("value1");
> + break;
> + case value3:
> + printf("value3");
> + case value2:
> + printf("value2");
> + break;
> + default:
> + break;
> + }
> +}
> +
> +// CHECK: switch
> +// CHECK: label
> +// CHECK: label
> +// CHECK: ]
> +// CHECK: call
> +// CHECK: call
> +// CHECK: call
> +// CHECK: ret void
> +void bar(int i) {
> + switch(i) {
> + case value1:
> + printf("value1");
> + break;
> + case value2:
> + printf("value2");
> + case value3:
> + printf("value3");
> + break;
> + default:
> + break;
> + }
> +}
> +
> +// CHECK: switch
> +// CHECK: label
> +// CHECK: label
> +// CHECK: ]
> +// CHECK: call
> +// CHECK: call
> +// CHECK: ret void
> +void baz(int i) {
> + switch(i) {
> + case value1:
> + printf("value1");
> + break;
> + case value2:
> + case value3:
> + printf("value23");
> + break;
> + default:
> + break;
> + }
> +}
> +
> +// CHECK: br i1 true
> +// CHECK: br i1 false
> +int main(int argc, char**) {
> + foo(argc);
> + bar(argc);
> + baz(argc);
> + if(argc != value3)
> + printf("true");
> + if(argc == value3)
> + printf("false");
> + return 0;
> +}
> Index: utils/TableGen/ClangAttrEmitter.cpp
> ===================================================================
> --- utils/TableGen/ClangAttrEmitter.cpp
> +++ utils/TableGen/ClangAttrEmitter.cpp
> @@ -2186,7 +2186,8 @@
> Field = 1U << 12,
> CXXMethod = 1U << 13,
> ObjCProtocol = 1U << 14,
> - Enum = 1U << 15
> + Enum = 1U << 15,
> + EnumConstant = 1U << 16
> };
> uint32_t SubMask = 0;
>
> @@ -2221,6 +2222,7 @@
> .Case("Field", Field)
> .Case("CXXMethod", CXXMethod)
> .Case("Enum", Enum)
> + .Case("EnumConstant", EnumConstant)
> .Default(0);
> if (!V) {
> // Something wasn't in our mapping, so be helpful and let the developer
> @@ -2240,6 +2242,7 @@
> case Param: return "ExpectedParameter";
> case Class: return "ExpectedClass";
> case Enum: return "ExpectedEnum";
> + case EnumConstant: return "ExpectedEnumConstant";
> case CXXMethod:
> // FIXME: Currently, this maps to ExpectedMethod based on existing code,
> // but should map to something a bit more accurate at some point.
>
~Aaron
More information about the cfe-commits
mailing list