Loop Swapping #3

yukatan1701 · 2019-12-26T15:36:06Z

No description provided.

yukatan1701 · 2020-05-19T12:47:39Z

Added:

loop swapping transformation;
finding the range of the array in memory.

kaniandr

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

Нужно сделать rebase origin/master вместо merge.
Желательно разбить этот pull request на два, один с анализом приватных массивов и второй с преобразованием цикла, т.к. анализ это отдельная задача, которая влияет на весь проект, а не только на один проход преобразования.

kaniandr · 2020-05-24T21:03:34Z

lib/Analysis/Memory/DefinedMemory.cpp

@@ -624,8 +678,13 @@ void ReachDFFwk::collapse(DFRegion *R) {
            break;
        }
      }
-      if (!RS->getIn().MustReach.contain(Loc) && !(StartInLoop && EndInLoop))
-        DefUse->addUse(Loc);
+      if (!RS->getIn().MustReach.contain(Loc) && !(StartInLoop && EndInLoop)) {


Давайте пока добавим проверку в 653 строке о том, нашел ли find() участок памяти. Если не нашел, то if-блок перед этой строкой (681) можно игнорировать, т.к. он только делает анализ более точным и на консервативность не повлияет.

Поиск в find() поправить можно, но это нужно делать аккуратно, чтобы не поломать что-нибудь еще, я подумаю над этим, и когда исправлю вам напишу. Но думаю проверка здесь все равно не будет лишней.

Теоретически такой же find() в каком-то другом месте анализатора тоже может приводить к падению, надеюсь на не очень сложных примерах это не возникнет.

На том примере, что вы прислали, с добавленной проверкой у меня работает.

kaniandr

Я постарался детально посмотреть исходный кода, отметил замечания, предложения и вопросы в коде. Общие рекомендации ниже.

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

Для этого используется метод toDiag(...), какое сообщение выводить определяется через пространство имен diag, например, diag::warn_remove_directive_in_include, чтобы добавить новые сообщения нужно отредактировать файл Support/DiagnosticKinds.inc. Если в текст сообщения добавить числовые идентификаторы "... %0", то они будут заменены параметрами после toDiag(), например toDiag() << "X" напечатает "... X".
Диагностики должны быть добавлены для всех ситуаций отказа от преобразования, а также некорректной расстановки прагм.

Надо добавить тесты в репозиторий. Для автоматизации тестирования в анализаторе используется инструмент pts.

Запуск выглядит:

cd <path-to-tsar-trunk>/transform/<directory_with_tests>
pts.pl -qs -I <path-to-tsar-trunk>/utils -T . check - запуск всех тестов
pts.pl -qs -I <path-to-tsar-trunk>/utils -T . init  - генерация эталонных результатов для тестов
pts.pl -qs -I <path-to-tsar-trunk>/utils -T . <test-name> - запуск конкретного теста
pts.pl -qs -I <path-to-tsar-trunk>/utils -T . <test-name>.init - запуск теста, но вместо проверки выполнится обновление эталонного результата

Тест состоит из

Исходного кода теста
Файлов с результатами преобразования
Двух конфигурационных файлов (*.conf - для запуска и *.init.conf - для инициализации).

init и check в командах выше - это просто файлы, содержащие список всех конфигурационных файлов (init - содержит .init, check - , .conf указывать в имени не нужно).

Для запуска tsar должен быть доступен в PATH.
Сам скрипт написан на perl, соответственно Perl должен быть установлен. При первом запуске, может сказать, что не хватает каких-то модулей perl, тогда нужно установить эти модули вручную. Обычно он выдает команду в качестве подсказки, как устанавливать.

Примеры .conf и .init.conf файлов можно найти например в tsar/transform/inline.
Файл .init.conf содержит команду запуска анализатора, например так

...
suffix = tfm
sample = $name.c
sample_diff = $name.$suffix.c
options = '-clang-copy-propagation -output-suffix=$suffix'
run = 'tsar $sample $options'

Т.е. запустить tsar с опциями $options, над входным файлом $sample, положить результат в $name.tfm.c и сравнить с эталоном в файле $sample_diff.
Кроме того при инициализации теста в конец исходного файла помещается выдача в стандартный поток (например, диагностики) с префиксом //CHECK. Поэтому нужно сначала создать исходный файл теста, затем выполнить инициализацию через pts либо с помощью файла init, либо явно указав test-name.init в качестве аргумента. А дальше уже можно делать проверки с помощью check.

Форматирование кода

В анализаторе принято именование переменных с большой буквы, а функций с маленькой. Приватные члены классов должны быть с префиксом m. Местами надо также отступы поправить, размер одного отступа 2 пробела (TSAR Coding Standards).

kaniandr · 2020-05-27T21:08:16Z

lib/Transform/Clang/LoopSwapping.cpp

+  }
+}
+
+bool ClangLoopSwapping::LoadDependenceAnalysisInfo(Function &F) {


Я добавил обертку, которая дает доступ к данным с сервера, можно использовать ее. Она фактически делает тоже самое, находится в Analysis/Memory/DIClientServerInfo.h. Имеет смысл использовать ее, чтобы не отслеживать изменения в доступе к серверу. Сейчас AnalysisSocketImmutableWrapper дает доступ не к конкретному сокету, а к контейнеру с доступными сокетами. Последний активированный сокет можно получить через AnalysisSocketInfo::getActiveSocket.

kaniandr · 2020-05-27T21:10:02Z

lib/Transform/Clang/LoopSwapping.cpp

+
+bool ClangLoopSwapping::runOnFunction(Function &F) {
+  mFunction = &F;
+  dbgs() << "\n[LOOP SWAPPING]: Function '" << F.getName() << "'.\n";


Если нужно, чтобы отладочные печати остались, то они должны быть помещены в макрос LLVM_DEBUG. В этом случае они будут доступны при сборке анализатора в отладочном режиме и запуске его с опцией -debug=..., в качестве параметра опции указывается строка из #define DEBUG_TYPE. В данном случае не имеет смысла печатать факт начала обработки функции, т.к. такая функциональность доступна для любых проходов по опции -debug-pass=Executions

kaniandr · 2020-05-27T21:40:35Z

lib/Transform/Clang/LoopSwapping.cpp

+    ClangLoopSwappingServerProvider>;
+
+/// This analysis server performs transformation-based analysis.
+class ClangLoopSwappingServer final : public AnalysisServer {


Я добавил сервер анализа по-умолчанию, можно использовать его, для него больше не требуется выполнять initializeProviderOnServer(), для создания сервера по-умолчанию используется метод createDIMemoryAnalysisServer() он определен в Analysis/Memory/Passes.h. Как и раньше сервер создается при определении PassGroupInfo. При использовании этого сервера может понадобится дополнительный вызов createAnalysisWaitServerPass() в addBeforePass() (пример в Transform/Clang/SharedMemoryAutoPar.cpp.

kaniandr · 2020-05-27T21:48:48Z

lib/Transform/Clang/LoopSwapping.cpp

+
+  bool TraverseCompoundStmt(CompoundStmt *S) {
+    if (IsInScope() && mForStack == nullptr) {
+      mForStack = new std::stack<ForStmt *>;


Безопаснее использовать std::unique_ptr<...>. Этот стек используется, чтобы обработать только самые верхние циклы в гнезде, разве не достаточно для этого просто флага? При вложенной расстановке директив, внутренние директивы игнорируются? Возможно ли реализовать преобразование для обоих гнезд циклов сразу?

#pragma spf transform swap { for (...) { #pragma spf transform swap { for(...) {} for(...) {} } } for(...) {} }

kaniandr · 2020-05-28T07:34:47Z

lib/Transform/Clang/LoopSwapping.cpp

+      dbgs() << "[LOOP SWAPPING]: Too few loops for swap. Ignore.\n";
+      continue;
+    }
+    if (ranges.size() > 2) {


Если переставлены могут быть только 2 соседних цикла, то имеет смысл использовать std::vector<std::pair<>> для хранения списка, а не вложенные вектора, которые дороже с точки зрения производительности, в том числе копирования, и потребляемой памяти. В LLVM есть еще llvm::SmallVector<...>, который выделяет память статически заданного размера, а если ее не хватает, то ведет себя как std::vector. Например, llvm::SmallVector<Loop *, 2> не будет делать выделения памяти из кучи, если количество элементов не больше двух. Можно его использовать, если планируется делать перестановку большего количества циклов, например, пользователь мог бы задавать для каждого цикла идентификатор, а в директиве указывать новый порядок циклов.

kaniandr · 2020-05-28T07:40:03Z

lib/Transform/Clang/LoopSwapping.cpp

+        auto *Red1 = (**I1).get<trait::Reduction>();
+        auto Kind0 = Red0->getKind(), Kind1 = Red1->getKind();
+        if (!Red0 || !Red1 || Kind0 == trait::DIReduction::RK_NoReduction ||
+            Kind1 == trait::DIReduction::RK_NoReduction) {


Если стоит RK_NoReduction это значит, что анализатор не смог определить вид редукции и он может быть любым, поэтом преобразовывать в этом случае нельзя.

kaniandr · 2020-05-28T07:40:53Z

lib/Transform/Clang/LoopSwapping.cpp

+        if (!Red0 || !Red1 || Kind0 == trait::DIReduction::RK_NoReduction ||
+            Kind1 == trait::DIReduction::RK_NoReduction) {
+          dbgs() << "[LOOP SWAPPING]: No Reduction\n";
+          return true;


Странно, что здесь сразу стоит return, ведь к этому моменту возможно еще не все редукции проверены.

Suggested change

return true;

continue;

kaniandr · 2020-05-28T07:44:12Z

lib/Transform/Clang/LoopSwapping.cpp

+    for (auto TS1: traits1) {
+      auto *node1 = const_cast<DIAliasNode *>(TS1->getNode());
+      MemoryDescriptor Dptr1 = *TS1;
+      if (STR.isEqual(node0, node1) && Dptr0.is<trait::Reduction>() &&


Для проверки на равенство достаточно использовать ==, можно не строить остовное дерево (STR).

Suggested change

if (STR.isEqual(node0, node1) && Dptr0.is<trait::Reduction>() &&

if (node0 == node1 && Dptr0.is<trait::Reduction>() &&

kaniandr · 2020-05-28T07:49:18Z

lib/Transform/Clang/LoopSwapping.cpp

+  if (!HasSameReductionKind(traits0, traits1, STR)) {
+    dbgs() << "[LOOP SWAPPING]: Failed to swap loops: different reduction kinds.";
+    return false;
+  }
+  if (HasTrueOrAntiDependence(traits0, traits1, STR)) {
+    dbgs() << "[LOOP SWAPPING]: Failed to swap loops: true or anti depencende.";
+    return false;
+  }


Достаточно ли этих двух проверок, что будет в случае:

for(...) { int X = ... // X - last private (second to last private) } for(...) { ... = X; // X - read only (shared, first private) }

А что будет, если между циклами есть какой-то код и они не идут подряд друг за другом?

kaniandr · 2020-05-28T07:52:10Z

include/tsar/Transform/Clang/LoopSwapping.h

@@ -0,0 +1,90 @@
+//=== LoopSwapping.h - Loop Swapping (Clang) ----*- C++ -*===//


Можно все перенести в LoopSwapping.cpp в безымянный namespace и отказаться от .h файла, т.к. ваш проход всегда запускается явно, через create..., и getAnalysis<...> для него использовать не предполагается, поэтому доступ к объявлению прохода в виде class ... в данном случае не требуется.

kaniandr

А есть ли тесты для прохода?

kaniandr · 2020-10-18T19:51:28Z

lib/Transform/Clang/LoopSwapping.cpp

+//
+//                       Traits Static Analyzer (SAPFOR)
+//
+// Copyright 2018 DVM System Group


Suggested change

// Copyright 2018 DVM System Group

// Copyright 2020 DVM System Group

kaniandr · 2020-10-18T20:23:31Z

lib/Transform/Clang/LoopSwapping.cpp

+    auto Res = RecursiveASTVisitor::TraverseStmt(S);
+    return Res;


Suggested change

auto Res = RecursiveASTVisitor::TraverseStmt(S);

return Res;

return RecursiveASTVisitor::TraverseStmt(S);

kaniandr · 2020-10-18T20:47:00Z

lib/Transform/Clang/LoopSwapping.cpp

+  }
+
+  bool TraverseCompoundStmt(CompoundStmt *S) {
+    if (mState == TraverseState::PRAGMA) {


Должна быть проверка того, что составной оператор идёт сразу за программой, а также того что нет висящих программ без составного оператора. Любые не корректные способы задания программы должны отлавливаться.

kaniandr · 2020-10-18T20:51:45Z

lib/Transform/Clang/LoopSwapping.cpp

+  bool TraverseForStmt(ForStmt *S) {
+    if (mState == TraverseState::OUTERFOR) {
+      auto Match = mLoopInfo.find<AST>(S);
+      if (Match != mLoopInfo.end()) {


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

kaniandr · 2020-10-18T21:07:17Z

lib/Transform/Clang/LoopSwapping.cpp

+    LLVM_DEBUG(dbgs() << "[LOOP SWAPPING]: no pragma found.\n");
+    return false;
+  }
+  Visitor.printLocations();


Если это отладочная печать, стоит весь вызов поместить в макрос LLVM_DEBUG, вместо того чтобы использовать макросы в теле функции.

kaniandr · 2020-10-18T21:28:16Z

lib/Transform/Clang/LoopSwapping.cpp

+  auto Traits0 = getLoopTraits(LoopID0);
+  auto Traits1 = getLoopTraits(LoopID1);


Правильно лета, что здесь используется идентификатор цикла на клиенте они на сервере? Откуда загружаются результаты анализа?

kaniandr · 2020-10-18T21:36:21Z

lib/Transform/Clang/LoopSwapping.cpp

+    const DIAliasTraitVector &TV0, const DIAliasTraitVector &TV1) const {
+  for (auto &TS0: TV0) {
+    auto *Node0 = TS0->getNode();
+    MemoryDescriptor Dptr0 = *TS0;


Зачем постоянно заводятся новые переменные, слишком много синтаксического сахара перегружает код, кроме, того компилятор конечно скорее всего все это уберет, но всё-таки это явное копирование объекта, пусть он всего лишь число, но число большое. С точки зрения читабельности это ненамного лучше, так как как объект TS также обладает методом is, как и дескриптор.

kaniandr · 2020-10-18T21:38:52Z

lib/Transform/Clang/LoopSwapping.cpp

+    for (auto &TS1: TV1) {
+      auto *Node1 = TS1->getNode();
+      MemoryDescriptor Dptr1 = *TS1;
+      if (Node0 == Node1) {


Вершины могут и не совпадать они могут быть всего лишь достижимой в дереве псевдонимов.

kaniandr · 2020-10-18T21:39:45Z

lib/Transform/Clang/LoopSwapping.cpp

+      auto *Node1 = TS1->getNode();
+      MemoryDescriptor Dptr1 = *TS1;
+      if (Node0 == Node1) {
+        if (Dptr0.is<trait::Readonly>() && !Dptr1.is<trait::Readonly>()) {


А если в обоих циклах объект не имеет тип readonly, а вызывает зависимость. И не надо ли здесь игнорировать редукционные переменные.

kaniandr · 2020-10-18T21:46:33Z

lib/Transform/Clang/LoopSwapping.cpp

+  auto &PragmaLevels = Visitor.getPragmaLevels();
+  auto &PragmaLoopsInfo = Visitor.getPragmaLoopsInfo();
+  for (auto it = PragmaLevels.rbegin(); it != PragmaLevels.rend(); it++) {
+    for (auto &Pragma : *it) {
+      auto PragmaItr = PragmaLoopsInfo.find(Pragma);


А зачем такая сложная конструкция, зачем хранить список директив отдельно от циклов связанных с каждой директивой и в итоге выполнять поиск в мапе. Разве нельзя сохранить список циклов, которые нужно переставить прямо в векторе с уровнями директив, избежав при этом лишних копирований из стека, содержащего списки циклов в мап в визиторе. Можно было бы сразу писать в объект в векторе уровней директив, ведь в момент обработки цикла вы знаете какую директиву в этом векторе вы обрабатываете.

…t comparison of reduction types.

…ing.

…sts.

yukatan1701 closed this May 19, 2020

yukatan1701 reopened this May 19, 2020

kaniandr reviewed May 24, 2020

View reviewed changes

kaniandr added the enhancement New feature or request label May 24, 2020

yukatan1701 force-pushed the swaploops branch from e8ac7f4 to 0512966 Compare May 26, 2020 17:45

kaniandr reviewed May 28, 2020

View reviewed changes

yukatan1701 force-pushed the swaploops branch 3 times, most recently from 4e98772 to 5e9ae24 Compare September 29, 2020 18:04

kaniandr requested changes Oct 18, 2020

View reviewed changes

yukatan1701 added 8 commits December 13, 2020 19:17

[TSAR, Transform] Create loop swapping tranformation.

1b0bf7d

[TSAR, Transform] Add checks of loop for the possibility of swapping.

88eeb16

[TSAR, Transform] Fix invalid client-server interaction, fix incorrec…

755eac2

…t comparison of reduction types.

[TSAR, Transform] Add handling of nested pragmas.

db422be

[TSAR, Transform] Change data structures and handle more cases.

f6e3de2

[TSAR, Transform] Limit scopes of loops for swapping.

2408863

[TSAR, Transform] Limit statements in pragma

dc4fead

[TSAR, Transform] Change traversal of pragma loops.

c03f3eb

yukatan1701 force-pushed the swaploops branch from 4811668 to c03f3eb Compare December 13, 2020 13:18

yukatan1701 added 2 commits December 19, 2020 00:15

[TSAR, Transform] Update the check for the availability of loop swapp…

f34d6ad

…ing.

[TSAR, Transform] Update diagnostic info for loop swapping and add te…

4e86664

…sts.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop Swapping #3

Loop Swapping #3

yukatan1701 commented Dec 26, 2019

yukatan1701 commented May 19, 2020

kaniandr left a comment

kaniandr May 24, 2020

kaniandr left a comment

kaniandr May 27, 2020

kaniandr May 27, 2020

kaniandr May 27, 2020

kaniandr May 27, 2020

kaniandr May 28, 2020

kaniandr May 28, 2020

kaniandr May 28, 2020

kaniandr May 28, 2020

kaniandr May 28, 2020

kaniandr May 28, 2020

kaniandr left a comment

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

kaniandr Oct 18, 2020

	if (STR.isEqual(node0, node1) && Dptr0.is<trait::Reduction>() &&
	if (node0 == node1 && Dptr0.is<trait::Reduction>() &&

		@@ -0,0 +1,90 @@
		//=== LoopSwapping.h - Loop Swapping (Clang) ----- C++ -===//

	// Copyright 2018 DVM System Group
	// Copyright 2020 DVM System Group

	auto Res = RecursiveASTVisitor::TraverseStmt(S);
	return Res;
	return RecursiveASTVisitor::TraverseStmt(S);

		auto Traits0 = getLoopTraits(LoopID0);
		auto Traits1 = getLoopTraits(LoopID1);

Loop Swapping #3

Are you sure you want to change the base?

Loop Swapping #3

Conversation

yukatan1701 commented Dec 26, 2019

yukatan1701 commented May 19, 2020

kaniandr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaniandr left a comment

Choose a reason for hiding this comment

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

Надо добавить тесты в репозиторий. Для автоматизации тестирования в анализаторе используется инструмент pts.

Форматирование кода

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaniandr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment