Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split declaration statements #19

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Valeriya-avt
Copy link

No description provided.

@Valeriya-avt
Copy link
Author

Split declaration statements with ExternalRewriter (version 1).

Copy link
Member

@kaniandr kaniandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По поводу определения позиции типа в начале объявления.

Есть еще такая конструкция, как clang::TypeLoc, она позволяет отследить позиции типа в программе, т.е. можно описать, например, bool VisitTypeLoc(TypeLoc Loc) { ... } или TraverseTypeLoc.

Когда обрабатывается VarDecl обходится тип, который ее формирует от самого широкого к самому узкому, например, для int *, сначала будет посещен int *, а потом int.
Есть предположение, что если для VarDecl взять самый последний посещенный тип (TypeLoc), то это будет тот тип, который стоит перед объявлением, который общий для всех объявлений. И тогда, чтобы вычислить префикс, который соответствует типу можно взять диапазон от начала объявления до конца этого типа (именно от начала объявления, т.к. квалификаторы типа const не попадут в этот тип, но должны остаться).

Тогда должно быть все известно, что удалить лишнее и строки объявления.

Нужно еще обратить внимание, что не все объявления можно разбить, например,

struct {int X;} A, B;
A = B;

нельзя превратить в

struct {int X;} A,;
struct {int X;} B;
A = B;

т.к. в первом случае типы A и B одинаковые, а во втором уже нет.

Это относится как минимум к struct, class, union, enum.

Общие моменты

Сейчас как я понимаю допускаются директивы как на область видимости, так и на отдельный оператор? Можно так оставить. Но нужно проверять, что директива стоит в допустимом месте, т.е. нельзя ставить директиву перед любым оператором.

Хотелось бы, чтобы в конечно счете был отдельный метод, который можно было бы вызвать, чтобы обработать конкретное объявление переменных и разбить его. Тогда это позволит автоматически разбивать операторы объявления, при формировании параллельных версий программ в SAPFOR, когда это мешает поставить директиву распараллеливания. Возможно даже внутри этого метода стоит запускать отдельный Visitor, чтобы обойти все объявления в обрабатываемом операторе.

Т.е. есть два направления - разбить по директиве пользователя и по запросу от распараллеливателя в процессе генерации параллельной версии.

namespace llvm {
/// This pass separates variable declaration statements that contain multiple
/// variable declarations at once into single declarations.
class ClangSplitDeclsPass : public ModulePass, private bcl::Uncopyable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно перенести объявление прохода в .cpp файл, а .h файл вообще удалить. Т.к. проход преобразования, не может быть запрошен из другого прохода, то и интерфейс его стоит скрыть внутри .cpp файлы, оставив видимыми наружу только функции initialize... и create... объявленные в Passes.h.

Comment on lines 260 to 273
auto &TfmInfo = getAnalysis<TransformationEnginePass>();
auto *TfmCtx{TfmInfo ? TfmInfo->getContext(M) : nullptr};
if (!TfmCtx || !TfmCtx->hasInstance()) {
M.getContext().emitError("can not transform sources"
": transformation context is not available");
return false;
}
ASTImportInfo ImportStub;
const auto *ImportInfo = &ImportStub;
if (auto *ImportPass = getAnalysisIfAvailable<ImmutableASTImportInfoPass>())
ImportInfo = &ImportPass->getImportInfo();
auto &GIP = getAnalysis<ClangGlobalInfoPass>();
ClangSplitter Vis(*TfmCtx, *ImportInfo, GIP.getRawInfo());
Vis.TraverseDecl(TfmCtx->getContext().getTranslationUnitDecl());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это устаревший способ получить TransformationContext, он не будет работать с многофайловыми проектами. Сейчас более правильный способ почти нигде не используется в master, но с последующим большим обновлением, которое будет в конце этого кода, или начале следующего, это устаревший способ перестанет работать.
Правильный вариант для модульного прохода примерно такой:

  auto &TfmInfo{getAnalysis<TransformationEnginePass>()};
  if (!TfmInfo) {
    M.getContext().emitError("cannot transform sources"
                             ": transformation context is not available");
    return false;
  }
  ASTImportInfo ImportStub;
  const auto *ImportInfo = &ImportStub;
  if (auto *ImportPass = getAnalysisIfAvailable<ImmutableASTImportInfoPass>())
    ImportInfo = &ImportPass->getImportInfo();
  auto &GIP{getAnalysis<ClangGlobalInfoPass>()};
  auto *CUs{M.getNamedMetadata("llvm.dbg.cu")};
  for (auto *MD : CUs->operands()) {
    auto *CU{cast<DICompileUnit>(MD)};
    auto *TfmCtx{
        dyn_cast_or_null<ClangTransformationContext>(TfmInfo->getContext(*CU))};
    if (!TfmCtx || !TfmCtx->hasInstance()) {
      M.getContext().emitError("cannot transform sources"
                               ": transformation context is not available");
      return false;
    }    
    // Здесь уже может быть ваш визитор ClangSplitter.
    // Объемлющий цикл фактически обходит все файлы,
    // входящие в проект, и для каждого получает свой контекст.
  }

Comment on lines 84 to 85
bool isNotSingleFlag = false;
bool isAfterNotSingleFlag = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Глобальные переменные - это плохо, переменные должны быть членами класса.

bool TraverseDeclStmt(DeclStmt *S) {
bool tmp;
if(!(S->isSingleDecl())) {
if (!isNotSingleFlag)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А в каком случае isNotSingleFlag будет true в этой точке?

}

bool TraverseDeclStmt(DeclStmt *S) {
bool tmp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool tmp;

SourceRange varDeclRange(S->getBeginLoc(), S->getEndLoc());
if (varDeclsNum == 1) {
SourceRange toInsert2(Range.getBegin(), S->getEndLoc());
txtStr = Canvas.getRewrittenText(varDeclRange).str();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде бы нет причин делать txtStr членом класса, она может быть локальной для метода.

}
}

// bool VisitVarDecl(VarDecl *S) { // to traverse the parse tree and visit each statement
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Объявление:

int (*f)(int N, int(*C)[N]), x, (*restrict Q)[100], C[200][300];

Типы:

DeclStmt 0x55df67170ac8 <line:3:3, col:66>
          |-VarDecl 0x55df67170668 <col:3, col:29> col:9 f 'int (*)(int, int (*)[N])'
          |-VarDecl 0x55df671706e8 <col:3, col:32> col:32 x 'int'
          |-VarDecl 0x55df67170898 <col:3, col:52> col:46 Q 'int (*restrict)[100]'
          `-VarDecl 0x55df67170a38 <col:3, col:65> col:55 C 'int [200][300]'

Не получится вставить имя сразу после типа, например, для переменной f (функция).
Не получится вставить имя на место последнего пробела, например, для переменной Q (массив C99).

Comment on lines 129 to 195
if (isNotSingleFlag) {
SourceRange toInsert(notSingleDeclStart, notSingleDeclEnd);
mRewriter.RemoveText(toInsert, RemoveEmptyLine);

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удаление исходного объявления лучше перенести поближе к обработке конкретного оператора объявления, например, делать это в TraverseDeclStmt, сразу как оператор был обработан.

Comment on lines 144 to 209
bool HasMacro = false;
for_each_macro(S, mSrcMgr, mContext.getLangOpts(), mRawInfo->Macros,
[&HasMacro, this](clang::SourceLocation Loc) {
if (!HasMacro) {
toDiag(mContext.getDiagnostics(), Loc,
tsar::diag::warn_splitdeclaration_macro_prevent);
HasMacro = true;
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нет смысла проверять отсутствие макросов во все области видимости, т.к. обработка делается только для единичных операторов объявления и только, если эти операторы содержат макрос, то нужно запрещать их обработку.

Сейчас кстати судя по всему этой проверки нет, если прагма стоит на один оператор а не на область видимости.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все еще актуально.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрала проверку на отсутствие макросов во всей области видимости

@kaniandr kaniandr added the enhancement New feature or request label Nov 27, 2021
@Valeriya-avt
Copy link
Author

Добавила TraverseTypeLoc. В этой версии берётся самый последний посещенный тип и добавляется перед всеми объявлениями переменных. Такой подход не пригоден для некоторых типов, в частности для структур. У Type есть метод isStructureType(). Можно попробовать обрабатывать такие типы отдельно, но даже в таком случае брать первый, а не последний посещённый тип будет нельзя (пример: struct S * x, y).

@kaniandr
Copy link
Member

kaniandr commented Dec 3, 2021

Добавила TraverseTypeLoc. В этой версии берётся самый последний посещенный тип и добавляется перед всеми объявлениями переменных. Такой подход не пригоден для некоторых типов, в частности для структур. У Type есть метод isStructureType(). Можно попробовать обрабатывать такие типы отдельно, но даже в таком случае брать первый, а не последний посещённый тип будет нельзя (пример: struct S * x, y).

Есть тип clang::ElaboratedTypeLoc - это как раз конструкции вида struct S или X::Y. Можно проверить, что Loc это такой тип if (Loc.getAs<ElaboratedTypeLoc>()) ... и брать его, а не тип который ниже.

Брать диапазон Loc.getSourceRage() не совсем правильно, т.к. квалификаторы, например, const не попадут в этот диапазон. Например, для const int X; диапазон указывает только на int. По всей видимости в качестве начала диапазона надо брать начало оператора объявления переменной, а вот конец уже брать из TypeLoc.

@Valeriya-avt
Copy link
Author

Реализация с ElaboratedTypeLoc. Теперь разделение объявлений работает со структурами и константами. Также была проблема с разделением объявлений массивов, сейчас она исправлена. Исправлена некорректная обработка прагмы, теперь разделение переменных происходит только в случае наличия строки #pragma spf transform splitdeclaration.

@Valeriya-avt
Copy link
Author

Устаревший способ получения TransformationContext заменён на новый. Убраны глобальные переменные, теперь их роль играют переменные, являющиеся членами класса. Удалены переменные и функция, которые больше не используются.

@Valeriya-avt
Copy link
Author

Исправлена некорректная работа программы на примерах с несколькими составными операторами объявления переменных.

@Valeriya-avt
Copy link
Author

Реализация разделения на одиночные составных операторов инициализации переменных.

Copy link
Member

@kaniandr kaniandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Общие замечания:

  • Форматирование кода: длина строк, правила именования типов и переменных, закрытых членов класс и т.д. (подробнее в TSAR Wiki).
  • Для всех ситуаций, которые не обрабатываются должно быть добавлено диагностическое сообщение о причине, почему преобрзование невозможно.
  • Нужно предусмотреть возможность, которая позволила бы ограничить множество преобразуемых объявлений. Можно считать, что будет какой-то дополнительный проход, к оторому будет обращаться ваш проход и у которого он получить список VarDecl, которые нужно вынести в отдельный оператор. Т.е. Нужно предсмотреть, что параметром вашего визитора будет llvm::DenseSet<VarDecl *>, и разделять нужно только эти объявления.

Comment on lines 6 to 8
DeadDeclsElimination.cpp Format.cpp OpenMPAutoPar.cpp
SharedMemoryAutoPar.cpp DVMHSMAutoPar.cpp StructureReplacement.cpp
LoopInterchange.cpp LoopReversal.cpp SplitDecls.cpp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надо поправить, в таком виде не собирается, видимо после устранения конфликтов при rebase могли продублироваться имена файлов.

Vis.TraverseDecl(TfmCtx->getContext().getTranslationUnitDecl());
return false;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

if (auto *ImportPass = getAnalysisIfAvailable<ImmutableASTImportInfoPass>())
ImportInfo = &ImportPass->getImportInfo();
auto &GIP = getAnalysis<ClangGlobalInfoPass>();
//mGlobalInfo = &GIP.getGlobalInfo();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//mGlobalInfo = &GIP.getGlobalInfo();

Comment on lines 371 to 372
private:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private:
private:


private:

TransformationContext *mTfmCtx;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TransformationContext *mTfmCtx;
ClangTransformationContext *mTfmCtx;

Comment on lines 144 to 209
bool HasMacro = false;
for_each_macro(S, mSrcMgr, mContext.getLangOpts(), mRawInfo->Macros,
[&HasMacro, this](clang::SourceLocation Loc) {
if (!HasMacro) {
toDiag(mContext.getDiagnostics(), Loc,
tsar::diag::warn_splitdeclaration_macro_prevent);
HasMacro = true;
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все еще актуально.

SourceRange toInsert(S->getBeginLoc(), S->getEndLoc());
ProcessGlobalDeclaration(S, toInsert);
}
if (localVarDecls.isNotSingleFlag) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде бы если переменная глобальна, то эта проверка вообще не должна выполняться, не нужен ли здесь else if вместо if?

if (mGlobalInfo.findOutermostDecl(S)) {
if (globalVarDeclsMap[S->getBeginLoc()].varDeclsNum == 0) {
std::map<clang::SourceLocation, notSingleDecl>::iterator it;
for (it = globalVarDeclsMap.begin(); it != globalVarDeclsMap.end(); it++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Почему не использовать метод find()? Зачем использовать map, если поиск делается перебором его элементов?
  • Вместо std::map, лучше использовать llvm::DenseMap.
  • Применять многкратно оператор [...] для map нехорошо, каждое его применение - это поиск по в map, лучше найти однократно и затем использовать полученную ссылку/итератор на объект для доступа.
  • Для вставки удобно использовать метод try_emplace(), он сразу вернет пару итератор на новый объект или на уже существующий объект и флаг типа bool, указыавющий на то был ли объект добавлен или он уже был в контейнере.

localVarDecls.notSingleDeclEnd = S->getEndLoc();
varPositions[S->getBeginLoc()] = S->getEndLoc();
} else {
std::cout << "IS SINGLE\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В LLVM для вывода используются собственные потоки outs(), errs(), dbgs(): outs() << ... << .... В финальной версии не должно быть отладочной печати, либо ее можно оставить, но окружить специальным макросом LLVM_DEBUG: LLVM_DEBUG(dbgs() << "[SPLIT DECLARATIONS]: ...\n"). В этом случае отладочная печать будет доступна при сборке системы в Debug по опции -debug-only=clang-split-decls (имя опции определяется макросом `#define DEBUG_TYPE "clang-split-decls" в начале .cpp файла).

return type;
}

bool TraverseTypeLoc(TypeLoc Loc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне казалось раньше была отдельная обработка структурных типов?
Сейчас struct {int X;} A, B; обрабатывается неправильно.
А что должно быть для такого случая: int (*f)(int N, int(*C)[N]), x, (*restrict Q)[100], C[200][300];, сейчас он не преобразуется.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Корректно обрабатывается struct S {int X;} A, B;. Ранее мы решили случаи типа int (*f)(int N, int(*C)[N]), x, (*restrict Q)[100], C[200][300]; не обрабатывать, такие объявления отлавливаются при помощи bool VisitParmVarDecl(ParmVarDecl *S)

@Valeriya-avt
Copy link
Author

Проведён рефакторинг кода в соответствии с правилами оформления кода. Для ситуаций, которые не обрабатываются, добавлены диагностические сообщения. Устранены многократные применения оператора [...] для map. Внесены изменения в обработку составных объявлений глобальных переменных.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants