[PATCH] D73498: [NFC] Remove extra headers included in Loop Unroll and LoopUnrollAndJam files

[Anh Tuyen Tran via Phabricator via llvm-commits]

anhtuyen marked 3 inline comments as done.
anhtuyen added a comment.

In D73498#1848429 <https://reviews.llvm.org/D73498#1848429>, @rnk wrote:

> I don't have time to review all these headers, but I looked at 3, and 2/3 are used, and would cause build errors if they did not happen to be pulled in by transitive includes. Generally, code should include what it uses, so 2/3 of the changes I looked at seem undesirable.
>
> If you want to meaningfully make the build faster, I would do it like this:
>
> - extract the command line to recompile the source file of interest (`rm lib/.../LoopUnrollAndJam.cpp.o` `ninja -v -n lib/.../LoopUnrollAndJam.cpp.o`) Paste it in a shell script to easily rerun.
> - compile the file, count the number of lines in the .d file to approximate how many includes it actually needed
> - remove one include from the source file
> - recompile, recount number of lines in .d file. If it did not get smaller, put the header back.
> - repeat.
>
>   In this way, you only ever make changes that actually make the build faster. It is also best to start by focusing on header files instead of cpp files, since they introduce transitive includes. Because of that, removing an include from a header has more impact on the overall build time.


Thanks, Reid @rnk for your planing out a detailed course of action. Below is my plan, and please let me know if you have objection.

1. Dave has done a very diligent review and I will further undelete (i.e. keep) all header files raised by him before committing the patch. I do not know the names of the 2/3 headers you think we better keep, but they will likely be kept based on Dave's comments. Since the chance of someone removes an include from a header file without checking if we can build is quite small,  the chances of having build errors is negligible too. This patch, though it is small, does remove many redundant headers which Dave has pointed out and thus it's worth it.
2. Using something similar to what you described above, I will put up another patch after I am done with my current project.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:29
 #include "llvm/IR/Dominators.h"
-#include "llvm/IR/IntrinsicInst.h"
-#include "llvm/IR/LLVMContext.h"
----------------
dmgreen wrote:
> There are some references to Intrinsics. Best to keep this one to be more robust.
Thank you, Dave @dmgreen ! I will un-delete Intrinsics accordingly.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:14
 
-#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
----------------
dmgreen wrote:
> We still use this for the moment (although may not in the future).
Thanks, I will put it back.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:29
 #include "llvm/IR/Dominators.h"
-#include "llvm/IR/IntrinsicInst.h"
-#include "llvm/IR/LLVMContext.h"
----------------
dmgreen wrote:
> Same as above, keep this one.
Yes, I will keep it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73498/new/

https://reviews.llvm.org/D73498