[PATCH] D126586: [InstrProf] Single byte counters in coverage

gulfem marked an inline comment as done. gulfem added inline comments. Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:1080-1081 +void CodeGenPGO::setProfileVersion(llvm::Module ) { + if (CGM.getCodeGenOpts().hasProfileClangInstr() && + llvm::EnableSingleByteCoverage) { +

[PATCH] D126586: [InstrProf] Single byte counters in coverage

gulfem updated this revision to Diff 42. gulfem added a comment. 1. Use || to merge counters that refer to the same regions 2. Use || to merge counters from raw profiles Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126586/new/

[PATCH] D126586: [InstrProf] Single byte counters in coverage

gulfem updated this revision to Diff 553992. gulfem added a comment. Fixed the issue that causes a crash in InstCombinePHI.cpp. Phi instructions need to be inserted at the beginning of basic blocks, and profile increments need to inserted after phis for conditional operators. Repository: rG

[PATCH] D126586: [InstrProf] Single byte counters in coverage

gulfem updated this revision to Diff 551687. gulfem added a comment. Remove hyphen between "single counter" for consistency Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126586/new/ https://reviews.llvm.org/D126586 Files:

[PATCH] D126586: [InstrProf] Single byte counters in coverage

gulfem updated this revision to Diff 551686. gulfem added a comment. Ensure the correct traversal order for loops in CodeGenPGO to fix the profile test failures (Profile/cxx-hash-v2.cpp and Profile/cxx-rangefor.cpp) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

gulfem updated this revision to Diff 550163. gulfem added a comment. - Extend the prototype by implementing the rest of the control-flow statements - Rename the prototype to single byte counters to be consistent with the single byte coverage used in PGO Repository: rG LLVM Github Monorepo

[PATCH] D157944: [InstrProf] Single byte counters in coverage

gulfem created this revision. Herald added subscribers: wlei, ellis, wenlei. Herald added a project: All. gulfem requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This patch inserts 1-byte counters instead of an 8-byte counters into llvm

[PATCH] D127040: [InstrProf][WIP] Implement boolean counters in coverage

gulfem abandoned this revision. gulfem added a comment. Herald added a subscriber: wlei. Created a duplicate review by mistake. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127040/new/ https://reviews.llvm.org/D127040

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd7fa92126cc3: [clang] Apply -fcoverage-prefix-map reverse order (authored by gulfem). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem updated this revision to Diff 517400. gulfem added a comment. Extend the comment about the usage of CoveragePrefixMap. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757 Files:

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:211 + + /// Prefix replacement map for coverage. + llvm::SmallVector, 0> CoveragePrefixMap; MaskRay wrote: > MaskRay wrote: > > While adding a comment, clarify what coverage

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem updated this revision to Diff 517375. gulfem added a comment. Add text to document that coverage prefix map should be traversed in reverse order. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem updated this revision to Diff 517012. gulfem marked an inline comment as done. gulfem added a comment. Added more info into HelpText and reversed prefix map in the first place to incorporate coworker feedback. GCC uses a linked list to store the prefix mappings in reverse order:

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem marked 2 inline comments as done. gulfem added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1638 : CGM(CGM), SourceInfo(SourceInfo) { - CoveragePrefixMap = CGM.getCodeGenOpts().CoveragePrefixMap; + for (const auto &[From, To] :

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem updated this revision to Diff 516993. gulfem added a comment. Added a comment about reverse traversal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757 Files:

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

gulfem updated this revision to Diff 516989. gulfem added a comment. Addressed feedback and rebased. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757 Files: clang/include/clang/Basic/CodeGenOptions.h

[PATCH] D148757: [clang] Apply last -fcoverage-prefix-map option

gulfem updated this revision to Diff 516853. gulfem added a comment. Updated the text. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757 Files: clang/include/clang/Basic/CodeGenOptions.h

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem added a comment. In D148757#4289152 , @MaskRay wrote: > In D148757#4289076 , @MaskRay wrote: > >> I'm creating a patch to change DebugPrefixMap to respect the command line >> option order... > > Created

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem updated this revision to Diff 516616. gulfem added a comment. Remove the changes from debuginfo, which is handled in D14897 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem added a comment. I compared Clang and GCC behavior. **1) Following user-specified prefix mapping order** GCC uses a linked list to store the prefix mappings, so it applies the prefix mappings based on the user-provided order.

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem updated this revision to Diff 515560. gulfem added a comment. Modified the test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148757/new/ https://reviews.llvm.org/D148757 Files: clang/include/clang/Basic/CodeGenOptions.h

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem added a comment. In D148757#4282002 , @MaskRay wrote: > Can you add example options in the description and say how this patch is > going to change it? > > I think `std::map` is intended so that for `-f*-prefix-map` values, if both > `a/b=...`

[PATCH] D148757: [clang] Follow user-provided order for prefix map

gulfem created this revision. Herald added a project: All. gulfem requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This patch uses llvm::MapVector instead of std::map to ensure that iteration order is the insertion order for file prefix

[PATCH] D145646: [clang][driver] Enable '-flto' on AVR

gulfem added a comment. In D145646#4213975 , @MaskRay wrote: > In D145646#4213547 , @gulfem wrote: > >> We started seeing a test failure in `avr-ld.c`. >> >>

[PATCH] D145646: [clang][driver] Enable '-flto' on AVR

gulfem added a comment. We started seeing a test failure in `avr-ld.c`. /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: // LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" "-plugin" {{.*}} "-plugin-opt=mcpu=atmega328" ^ :1:1:

[PATCH] D143304: [Coverage] Map regions from system headers

This revision was automatically updated to reflect the committed changes. Closed by commit rG2155195131a5: [Coverage] Map regions from system headers (authored by gulfem). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143304/new/

[PATCH] D143304: [Coverage] Map regions from system headers

gulfem created this revision. Herald added a project: All. gulfem requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Originally, the following commit removed mapping coverage regions for system headers:

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

gulfem added a comment. In D129755#3844059 , @aaronpuchert wrote: > In D129755#3843144 , @gulfem wrote: > >> We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. >>

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

gulfem added a comment. In D129755#3843206 , @aaronpuchert wrote: > In D129755#3843144 , @gulfem wrote: > >> We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. >>

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

gulfem added a comment. We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview I'm trying to verify with our team whether it is a false positive, but I

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

gulfem added a comment. In D135356#3840428 , @sammccall wrote: > Fixed by 4b53c00173163774d32125fbcae283a46a9a4b19 > I think. It fixed the test error that we are seeing, thanks!

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

gulfem added a comment. We are seeing the same error reported above: FormatTests: clang/lib/Lex/Lexer.cpp:1151: SourceLocation clang::Lexer::getSourceLocation(const char *, unsigned int) const: Assertion `Loc >= BufferStart && Loc <= BufferEnd && "Location out of range for this buffer!"'

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

gulfem added a comment. In D131528#3717509 , @shafik wrote: > In D131528#3717141 , @gulfem wrote: > >> Can somebody please clarify this? Is the following code results in >> `undefined` behavior? >> >> enum E1

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

gulfem added a comment. Can somebody please clarify this? Is the following code results `undefined` behavior? enum E1 {e11=-4, e12=4}; E1 x2b = static_cast(8); `constexpr E1 x2 = static_cast(8)` seems like `undefined`, so `-Wenum-constexpr-conversion` is triggered. We started seeing

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. Added the description in https://reviews.llvm.org/D131628 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90275/new/ https://reviews.llvm.org/D90275 ___ cfe-commits mailing list

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. In D90275#3712991 , @nlopes wrote: > In D90275#3712969 , @gulfem wrote: > >> In D90275#3712880 , @nlopes wrote: >> >>> In D90275#3707793

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. In D90275#3712880 , @nlopes wrote: > In D90275#3707793 , @nlopes wrote: > >> In D90275#3671983 , @gulfem wrote: >> >>> In D90275#3671019

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. In D90275#3671019 , @nlopes wrote: > Could you please add a description of this attribute to LangRef? > We need the semantics of this. > > Thank you! Sure, I'll do that! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

gulfem added a comment. In D126586#3646197 , @davidxl wrote: > For coverage mode, why using 'incrementProfileCounter'? Should it be set to > '1' instead? Also is the 'or' expression needed? The expression can be > folded to either zero or 1.

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

gulfem added a comment. @davidxl and @vsk, This is a WIP prototype implementation, and I would love to hear your early feedback on using 1-byte counters for coverage idea if possible. I'll continue extending the prototype, but that would be great to learn if there is anything fundamentally

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

gulfem updated this revision to Diff 434224. gulfem added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Describe the high-level changes to adding and subtracting counters. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127040: [InstrProf][WIP] Implement boolean counters in coverage

gulfem created this revision. Herald added subscribers: ellis, wenlei, hiraditya. Herald added a project: All. gulfem requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This patch inserts 1-byte boolean counters instead of

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. > Then pipe that to a file (note the -E I added at the end). You should get a > file that looks like some slightly-wonky C++ code. I got the following output after running it via `-E`.

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. In D119544#3486241 , @erichkeane wrote: > Ah shucks... Thanks for the heads up. Is there any chance to get you to get > me a pre-processed version of this failure to play with? I've not had luck > compiling/running libc++

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. We started seeing several test failures after this commit: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview One example is `nothrow_forward_range.compile.pass.cpp`. Script: -- : 'COMPILED WITH';

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem added a comment. @davidxl, I added comments in https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2 @MaskRay, I followed the style guide for nested if statements in https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569 + if (!containsProfilingIntrinsics(M)) { +if (!CoverageNamesVar || !NeedsRuntimeHook) { + return MadeChange; MaskRay wrote: >

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc7f91e227a79: [InstrProfiling] No runtime hook for unused funcs (authored by gulfem). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem added a comment. In D122336#3406843 , @vsk wrote: > So long as it doesn't change behavior expected by tapi on Darwin, I think > it's OK. I doubt any other platforms are similarly affected. I don't think it should impact TAPI because it relies on

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem added a comment. In D122336#3406186 , @vsk wrote: > Sorry for ay delayed replies - I've switched teams at Apple and find it > difficult to keep up with llvm reviews. > >> it's my understanding is that we might be generating coverage record for

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem added a comment. In D122336#3405835 , @davidxl wrote: > Should this be fixed in Clang (not generating coverage record)? Adding the > logic here is a little confusing. @davidxl, I originally did that (not generating coverage records) because I

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem updated this revision to Diff 417945. gulfem added a comment. Addressed phosek's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122336/new/ https://reviews.llvm.org/D122336 Files:

[PATCH] D121812: [clang][deps] NFC: De-duplicate clang-cl tests

gulfem added a comment. In D121812#3405060 , @jansvoboda11 wrote: > Sorry for the breakage and thanks for reporting this! This is a real bug > uncovered by your build using `/opt`. I have a fix here: D122385 > .

[PATCH] D121812: [clang][deps] NFC: De-duplicate clang-cl tests

gulfem added a comment. Newly added tests `ClangScanDeps/cl-output.c` and `ClangScanDeps/cl-xclang.c` started failing in our Mac toolchain builds: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b881168677577537/overview Here's the link to the log:

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

gulfem created this revision. Herald added subscribers: ellis, abrachet, phosek, hiraditya. Herald added a project: All. gulfem requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. CoverageMappingModuleGen generates a coverage

[PATCH] D118862: [clang][driver] add clang driver support for emitting macho files with two build version load commands

gulfem added a comment. The following two tests started failing in our bots: Clang :: Driver/darwin-ld-platform-version-target-version.c Clang :: Driver/darwin-zippered-target-version.c

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:132 + // Place new instruction sequence after GEP. + Builder.SetInsertPoint(GEP); + Value *Index = GEP->getOperand(2); jrtc27 wrote: > This line causes the bug

[PATCH] D103938: Diagnose -Wunused-value based on CFG reachability

gulfem added a comment. We also started seeing similar `libc++` test failures in our Fuchsia builds: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8835548361443044001/+/u/clang/test/stdout Failed Tests (89): libc++ ::

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

gulfem added a comment. > I'll commit this by the end of the day if @rsmith wasn't able to take a look. Sounds good, thank you very much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102531/new/ https://reviews.llvm.org/D102531

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

gulfem added a comment. Any update on this review? Our builds are still broken. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102531/new/ https://reviews.llvm.org/D102531 ___ cfe-commits mailing list

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

gulfem added a comment. We started seeing the following test failures in our Fuchsia builds after this patch: Failed Tests (10): libc++ :: libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp libc++ ::

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

gulfem added a comment. @aaron.ballman would it be possible for you to review this patch, so we can fix our broken builds? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102531/new/ https://reviews.llvm.org/D102531

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

gulfem added a comment. We recently enabled assertions in our Fuchsia builds, and our builds started to fail: FAILED: obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o ../../../recipe_cleanup/clangLhcV7J/bin/clang++ -MD -MF obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o.d

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73 + return false; + +// If an operand in the lookup table is not dso_local, pcc wrote: > In the version of the patch that you committed, you have this check

[PATCH] D102286: [HWASan] Build separate LAM runtime on x86_64.

gulfem added a comment. > Looks like the same issue I fixed in https://reviews.llvm.org/rGd97bab651185. > Let me know if you have the issue after that commit. Thanks, that resolved the build issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D102286: [HWASan] Build separate LAM runtime on x86_64.

gulfem added a comment. We started seeing test failures in our Windows builds in Fuchsia after this patch. FAILED: compiler-rt/lib/hwasan/CMakeFiles/RTHwasanAliases.aarch64.dir/hwasan_interceptors.cpp.o C:\b\s\w\ir\x\w\staging\llvm_build\.\bin\clang++.exe

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. > I would say if it is going to take longer than the end of the week to fix the > issue, it might be nice to have it reverted so that other people's builds > continue to work (I know a few people who build with full LTO in a two stage > configuration every week).

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. @nathanchance do you prefer me to revert the patch first as it might take me a while to investigate it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. > This patch breaks a two stage build with LTO: Thanks for the report @nathanchance, and I'm looking at it now. Is this failure from one of the bots? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

gulfem added a comment. > Thanks! We found the issue. I think I'll revert the patch, and will reland a > fixed version. Great, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92191/new/ https://reviews.llvm.org/D92191

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

gulfem added a comment. > @gulfem We're taking a look now. Could you please post a link to the full > build log, including the cmake flags used? This is the link to one of our failing builds: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang-linux-arm64/b8849674814150864912/overview Here,

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

gulfem added a comment. This seems to break our `Fucshia` builds. If we cannot do a quick fix, we need to revert this patch. Please see the error message below: FAIL: Clang :: ClangScanDeps/vfsoverlay.cpp (1863 of 27822) TEST 'Clang :: ClangScanDeps/vfsoverlay.cpp'

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. > FYI, I pushed the fix for the aarch64-coff issue now (D99572 > , rGd5c5cf5ce8d921fc8c5e1b608c298a1ffa688d37 > ) and > pushed another commit to remove the code for

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. > Would you be ok with reverting this change until I can sort that out, or can > we disable the pass for those targets until then? I will disable the pass for those targets for now. When the issue is resolved, I would like to enable it for those targets as well.

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added inline comments. Comment at: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn:64 "PromoteMemoryToRegister.cpp", +"RelLookupTableConverter.cpp" "SSAUpdater.cpp", thakis wrote: > leonardchan wrote: > > Good that you added this,

[PATCH] D94355: [Passes] Add relative lookup table converter pass

This revision was automatically updated to reflect the committed changes. Closed by commit rG78a65cd945d0: [Passes] Add relative lookup table converter pass (authored by gulfem). Changed prior to commit: https://reviews.llvm.org/D94355?vs=331139=332445#toc Repository: rG LLVM Github

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + lebedev.ri wrote: > gulfem wrote: > > lebedev.ri wrote: > > > 1. But all tests are using `x86_64`

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. @lebedev.ri do you have further comments? If not, I would like to submit this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355 ___

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem marked 2 inline comments as done. gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + lebedev.ri wrote: > 1. But all tests are using `x86_64`

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem marked 4 inline comments as done. gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:32 + // If lookup table has more than one user, + // do not generate a relative lookup table. + if (!GV.hasOneUse()) hans

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem updated this revision to Diff 331139. gulfem added a comment. 1. Add no_relative_lookup_table.ll test case that checks the cases where relative lookup table should not be generated 2. Add comments about dso_local check and single use check Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem marked 7 inline comments as done. gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:384 + bool shouldBuildRelLookupTables() { +const TargetMachine = getTLI()->getTargetMachine(); leonardchan wrote: > Sorry, I think

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem updated this revision to Diff 330405. gulfem added a comment. 1. Used IsConstantOffsetFromGlobal() function 2. Added this pass to the end of legacy pass manager pipeline 3. Moved all the tests under a new test directory Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem updated this revision to Diff 328354. gulfem added a comment. Use TTI hook for target machine checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355 Files:

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem closed this revision. gulfem added a comment. Since this patch has been merged and the issue in the test has been resolved, I'm closing this review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90275/new/ https://reviews.llvm.org/D90275

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:76-77 + + /// If lookup table has more than one user, + /// do not generate a relative lookup table. + if (!GlobalVar.hasOneUser()) leonardchan wrote: > What's the

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. In D94355#2572553 , @leonardchan wrote: > It looks like you have everything setup for running on the new PM, but it > doesn't look like the pass is added anywhere in the new PM pipeline. Thank you very much for pointing that out

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem added a comment. > Thanks for pushing this forward! I think this will be a nice transformation > once all the details are worked out. Thank you very much for all of your wonderful constructive feedback! I learned much more about LLVM and IR internals. Appreciate all your help!

[PATCH] D94355: [Passes] Add relative lookup table converter pass

gulfem marked 17 inline comments as done. gulfem added inline comments. Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2 +// Check switch to lookup optimization in fPIC and fno-PIC mode +// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S -emit-llvm

[PATCH] D94355: [Passes] Add relative lookup table generator pass

gulfem updated this revision to Diff 326265. gulfem added a comment. Herald added subscribers: wenlei, nikic, kerbowa, steven_wu, nhaehnle, jvesely. - Rename the pass to RelLookupTableConverter to be consistent - Addressed reviewers' feedback - Added tests for user-defined lookup tables and

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem added a comment. @lebedev.ri based on your feedback, I added it as a separate pass and added support for user-defined lookup tables. Please let me know if you have any comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem updated this revision to Diff 324144. gulfem marked an inline comment as not done. gulfem added a comment. Herald added a subscriber: mgorny. Implement it as a separate pass and apply it to user-defined lookup tables as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem marked an inline comment as done. gulfem added a comment. In D94355#2530225 , @lebedev.ri wrote: > Can you please add an explanation to the patch's description as to why > we don't want to instead convert non-relative/relative LUT's elsewhere, >

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem marked 9 inline comments as done. gulfem added inline comments. Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2 +// Check switch to lookup optimization in fPIC and fno-PIC mode +// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S -emit-llvm -o

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem updated this revision to Diff 320025. gulfem added a comment. 1. Simplified test cases and increased readibility of the tables 2. Added x86_64 and aarch64 check and tiny or small code modes check to ensure 32 offsets 3. Modified single value test case Repository: rG LLVM Github

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

gulfem updated this revision to Diff 318372. gulfem added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. 1. Apply relative lookup table generation in fPIC mode 2. Add a test case for single value case 3. Remove hard-coding 64 bit pointers 4. Improve

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. > I fixed it in > https://reviews.llvm.org/D92493 > But that was just to make my test pass. Ok, thank you! I did not realize that typo after the merge, and sorry about that! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. > The problem in `llvm/test/Bitcode/attributes.ll` is clear? Instead of `define void @f70() nocallback`, it should be `define void @f69() nocallback`, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90275/new/

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem added a comment. > This is missing a lang ref entry for `nocallback` and the `attributes.ll` > test is arguably broken (see below). Could you please elaborate on missing lang ref entry? Where that should be added? > The "definition" in `llvm/include/llvm/IR/Attributes.td` (see below),

[PATCH] D90275: [clang][IR] Add support for leaf attribute

gulfem updated this revision to Diff 310290. gulfem marked an inline comment as done. gulfem added a comment. Add a new line at the end of the test file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90275/new/ https://reviews.llvm.org/D90275

  1   2   >