- User Since
- Nov 16 2017, 6:22 PM (65 w, 5 d)
LGTM. Thanks for adding these.
Mon, Feb 18
Thu, Feb 14
Tue, Feb 12
Mon, Feb 11
LGTM. I had a few suggestions but nothing looks wrong to me.
Thu, Feb 7
I've updated the patch to replicate GNU's c++filt behavior. This patch now only splits strings that are passed via stdin. It does not split strings that are passed as arguments to llvm-cxxfilt via command line.
Wed, Feb 6
The test case in your patch definitely causes a problem, but that's because the -start-before/-stop-after is used. If I run that same test through llc, the UnreachableBlockElim pass kicks-in and drops the dead code.
I took a look at the code in your Janurary 24th comment. I was unable to get the same dead block to surface, but most likely I had the wrong flag set. How did you build the sample in that comment?
Tue, Feb 5
Mon, Feb 4
Fri, Feb 1
Thu, Jan 31
- Update two tests to use FileChecks --implicit-check-not
- Remove some cruft that, I think, was in an earlier version of this patch.
- Update the CHECK-COUNT test to simply perform a CHECK followed by a CHECK-NOT.
Wed, Jan 30
- I took a hint and decided to more the decision to print the section mapping with the program headers down into the dumper classes. This is similar to how the printSymbols and printDynamicSymbols are handled.
- Removed a test
- Replaced the uses of grep in the tests with CHECK-NOT and CHECK-COUNT
- Update a comment, it was bothering me.
- Updated the test to also look at the presentation of other sections/symbols, not just the debug sections.
- Replaced the use of an auto with the actual type.
- Rebased against master.
Tue, Jan 29
Thanks for the feedback, and my apologies for not originally providing as thorough of
testing as I should have.
- Moved the call to printSectionMapping after printSectionHeaders`
- Specifying --program-headers will still print the section mapping; however, the calls to printSectionMapping has been removed from the body of printProgramHeaders
Mon, Jan 28
Fri, Jan 25
My instinct is that the place you've made the change is too deep in the hierarchy, and that's why you have the problem in llvm-objdump. My feeling is that Symbol::getName() should probably return the name of the symbol, and leave it up to the caller to fall back and get the section name for section symbols.
Thu, Jan 24
- Added a comment to the change in ELFObjectFile.h.
- Updated the condition to retrieve the section name if the symbol is a section and has no name.
- I discovered that my original patch broke an aarch64 test. In that case, llvm-objdump was using the section name (produced from my patch) instead of ignoring the section in general. That had the effect of the section name being displayed in the disassembly instead of the aarch64/arm mapping name "$x." This patch now ignores section names and preserves the mapping name in that case. I'm not sure that the change to llvm-objdump is the best choice, but it doesn't seem to break any llvm tests or any test I generated while fiddling around with this patch.
I like this patch, and I like the DominatorTree clean up/refactor. As you pointed out, your patch does make the DT required, which makes sense because your solution must make use of that analysis. I don't see that as an issue. Overall, this patch makes sense to me, but I'm curious as to what others think.
Wed, Jan 23
I have a few changes to this patch. Namely, I want to avoid breaking the llvm-objdump/AArch64/elf-aarch64-mapping-symbols.test. I didn't realize I had broken that test earlier, but I caught it in my second round of testing. It turns out that llvm-objdump presents a data mapping symbol ($x) when the test works. However, with the patch above, that mapping symbol is gone and the section name is displayed instead. I'm working on fixing my patch to avoid that situation.
Tue, Jan 22
LGTM. I'd wait a bit to see if others have anything else to add.
Jan 10 2019
*Ping* to see if anyone else has any feedback towards this patch.
Jan 9 2019
- Moved the logic for controlling access to performPRE/performScalarPRE functions into a separate patch.
- Added an assertion that will trigger on 9 tests if the BlockRPONumber map is invalid. In particular, test/Transforms/GVN/nonescaping-malloc.ll, which also requires an assert build when testing.
Jan 8 2019
Update the SelectionDAG handling of the mca_code_region_start and mca_code_region_end intrinsics.
- Add a bool to GVN that tracks the status (valid or invalid) of the BlockRPONumber map.
- Use that flag to indicate that the BlockRPONumber is invalid when blocks are added to the function being analyzed.
- Make the BlockRPONumber map: DenseMap<AssertingVH<BasicBlock>, uint32_t>
Jan 7 2019
- Rebased against master
- Moved most of the added llvm-mca CodeGen/X86 tests to CodeGen/Generic.
Jan 4 2019
- Update the symbol name parsing for the llvm-mca region markers.
Jan 3 2019
This patch aims to clean-up two items in GVN.
Jan 2 2019
- Renamed the assembly comment that gets added to assembly files when using the llvm.mca.code.region.start intrinsic.
- Removed the check in IR/Verifier that rejected programs if llvm-mca code regions span multiple blocks.
In short, we let llvm-mca handle multiple blocks as it always has. To be fair here, llvm-mca doesn't handle branch instructions, but a user can currently place LLVM-MCA assembly comments such that the instructions cross multiple blocks.
Dec 21 2018
- Corrected some comments and removed unnecessary code, as a result of the last two patches.
Thanks for the feedback @efriedma.
Dec 20 2018
Dec 19 2018
Dec 18 2018
Dec 17 2018
- Following some discussion of the RFC on the mailing list, this patch makes a few improvements:
- Object files and executables are supported. This is accomplished by scanning the symbol table for mca_code_region_start and mca_code_region_end symbols.
- This solution does not rely on target specific relocations.
- Regions cannot be nested, so a start of region label/symbol must be followed by an end (this has always been the case).
- Symbol names are encoded with the user's defined region number." That number is just for cosmetic purposes and is only helpful for the user, llvm-mca can make use of that number to annotate its analysis reports.
- Added context
Dec 13 2018
Dec 12 2018
Thanks @courbet , these changes LGTM. I'd wait a day or two to see if anyone else has any feedback from the RFC on the mailinglist:
Dec 5 2018
Just pinging this patch and RFC to see if anyone else, aside from Andrea, has any feedback. The discussion on the RFC is here:
Dec 4 2018
Dec 3 2018
- Move the HasNoexcept lambda to its own static function.
- Added an additional check in HasNoexcept to recursively check return values.
- Modified the parameter check in Sema::CheckFunctionDeclaration to use llvm::any_of, all we need to know is if any of the parameters might have a mangling change.
- Updated the test to include the example provided by @Rakete1111.
@troyj, good catch! Thanks, and LGTM.
Nov 29 2018
Thanks for the reviews and added example. I'll update the logic to account for the return type, and add @Rakete1111 's example to the existing test.
Nov 28 2018
Thanks for answering my questions.
Nov 27 2018
LGTM, I'll let others weigh in. For the most part I think this patch is fine, as long as my questions do not point out any bugs.
Nov 26 2018
- Rebased this patch.
- Added an additional IRVerifier check to generate a compiler error if a llvm-mca code region belongs to more than one basic-block (this was discussed in the llvm-dev mailing list thread).