- User Since
- Oct 27 2012, 6:35 AM (298 w, 1 d)
Simplify regex even more.
The goal is to spare time of whoever will be adding some new scheduling model by explaining that the X86MCInstrAnalysis::isDependencyBreaking() needs to be modified too, for mca to be able to properly handle these dependency-breaking idioms.
It seems everyone else is busy with other differentials, and don't have time to spare to review this :/
Ping, i guess :)
Anything needed to get this going?
Nice! Some nits.
Anything to do to get this going? Or did this got replaced by some other differential? D48222?
(Removing from my review queue for the time being.)
The InstSimplify change needs it's own test set in test/Transforms/InstSimplify.
Other than nits i have no further comments, thanks.
I would prefer some more general solution, but this does not look awful, too.
You probably want to wait a bit in case others want to comment.
Can you add a comment to the end of the btver sched profile, that X86MCInstrAnalysis::isDependencyBreaking()
is also responsible (as in, needs to be modified) in detection of dep-breaking patterns?
Fri, Jul 13
Added all the sane negative tests.
Other than the one-use question i think this is ready for the review.
I have no further comments here.
I assume this works as intended; if not, since it is mainly a developer-only tool, further issues could be addressed later on.
Well, that's just great, with isCastPartOfExplictCast(), the ASTContext::getParents()
also does not return CXXStaticCastExpr as parent for such cases.
I don't know how to proceed.
Address @vsk review notes, although this will be revered by the next update dropping the faulty 'stack' optimization.
@vsk so yeah, no wonder that doesn't work.
Somehow in that test case ScalarExprEmitter::VisitExplicitCastExpr() never gets called.
(I'm pretty sure this worked with the naive implementation, so worst case i'll just revert the 'stack' code)
Trying to assess the issue..
Sorry about lack of the review, this kinda fell off my radar.
Address both test regressions with one fix - new TargetLowering hook.
FIXME: is there something preexising that i could use? I did look, and didn't find any..
Thu, Jul 12
Judging by the pattern, i would almost guess every single transform in instcombine will need such a fix?
Can this be generalized somehow, fixing the outer place which actually applies replacements?
Actually run $ ninja check-llvm this time, two tests regressed..
(The numbers won't be too representable, whole stage-1 takes ~40 minutes here...)
Ah I see, I'll run a few builds and take a stab at it, then.
Thank you for taking a look!
nounwind, fix some function names.
In general, looks good to me, but probably wait for someone else to review, too.
Filed https://bugs.llvm.org/show_bug.cgi?id=38149 to track the signed part of the pattern.
Not really. I agree that the last one is the best from IR clarity standpoint.
I was just looking for some other variants of this pattern.
(Sadly souper was of no help here, strangely.)
Thank you for the review.
Would be good if you could also put these folds into https://rise4fun.com/Alive and link them here,
to validate that at least the cases tested here are handled correctly.
Address @vsk's review notes.
- Maintain the stack of currently-being-visited CastExpr's
- Use that stack to check whether we are in a ExplicitCastExpr
- Move logic for deciding whether to emit the check out of EmitScalarConversion()
- Condense all overloads of EmitScalarConversion() down to one.
Wed, Jul 11
Add some more tricky tests where maintaining just the CastExpr part of AST stack would break them.
Address @spatel's review notes.
Tue, Jul 10
@vsk thank you for taking a look!
- Drop indeed-unneeded 'grouping' ImplicitCast check. For some reason i was very sure i just didn't add some macro, and there was grouping at this level, too.
- Added test with runtime blacklist, doubling as a test for -fno-sanitize-recover= test
- Check that sanitizer is actually enabled before doing the AST upwalk. I didn't measure, but it would be logical for this to be better.
Finished running it on a normal testset of my pet project.
- It fired ~18 times.
- There were no obvious false-positives (e.g. when an explicit cast was involved).
- At least 3 of those appear to be a true bugs.
- 4-5 more are probably bugs, but it is hard to tell.
- Last 10-11 appear to be mostly OK intentional truncating casts.
Mon, Jul 9
This broke the build:
$ ninja [0/17] Performing build step for 'libcxx_fuzzer_x86_64' ninja: no work to do. [2/17] Building CXX object projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_wait_release.cpp.o FAILED: projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_wait_release.cpp.o /usr/bin/clang++-6.0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Domp_EXPORTS -Iprojects/openmp/runtime/src -I/build/openmp/runtime/src -I/usr/include/libxml2 -Iinclude -I/build/llvm/include -I/build/openmp/runtime/src/i18n -I/build/openmp/runtime/src/include/50 -I/build/openmp/runtime/src/thirdparty/ittnotify -g0 -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -std=c++11 -O3 -g0 -fPIC -UNDEBUG -D _GNU_SOURCE -D _REENTRANT -fno-exceptions -fno-rtti -Wno-sign-compare -Wno-unused-function -Wno-unused-local-typedef -Wno-unused-value -Wno-unused-variable -Wno-switch -Wno-covered-switch-default -Wno-deprecated-register -Wno-gnu-anonymous-struct -Wno-unknown-pragmas -Wno-missing-field-initializers -Wno-missing-braces -Wno-comment -Wno-self-assign -Wno-vla-extension -Wno-format-pedantic -MD -MT projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_wait_release.cpp.o -MF projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_wait_release.cpp.o.d -o projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_wait_release.cpp.o -c /build/openmp/runtime/src/kmp_wait_release.cpp In file included from /build/openmp/runtime/src/kmp_wait_release.cpp:14: /build/openmp/runtime/src/kmp_wait_release.h:175:29: error: cast from 'volatile void *' to 'void *' drops volatile qualifier [-Werror,-Wcast-qual] KMP_FSYNC_SPIN_ACQUIRED(spin); ^ /build/openmp/runtime/src/kmp_wait_release.h:346:28: error: cast from 'volatile void *' to 'void *' drops volatile qualifier [-Werror,-Wcast-qual] KMP_FSYNC_SPIN_PREPARE(spin); ^ /build/openmp/runtime/src/kmp_wait_release.h:453:27: error: cast from 'volatile void *' to 'void *' drops volatile qualifier [-Werror,-Wcast-qual] KMP_FSYNC_SPIN_ACQUIRED(spin); ^ /build/openmp/runtime/src/kmp_wait_release.h:175:29: error: cast from 'volatile void *' to 'void *' drops volatile qualifier [-Werror,-Wcast-qual] KMP_FSYNC_SPIN_ACQUIRED(spin); ^
and several more pages of this.
If this can't be fixed soon (a hour?), *please* revert.
C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while balancing between getting drowned in everything being convertible,
and nothing being convertible. As usual, this comes with a price:
This is a compiler-rt part.
The clang part is D48958.
Sun, Jul 8
Can you rebase this since D48767 has landed?
I think those might be affected by the patch.
Sat, Jul 7
Rebased, just to control bitrot, no changes.
Rebased, just to control bitrot, no changes.
Fri, Jul 6
Some drive-by nits.
General observation: this is kinda large.
I would personally split split off the codegen part into a second patch.