- User Since
- Jul 29 2016, 12:33 AM (217 w, 5 h)
Tue, Sep 8
Mon, Sep 7
- clang-tidy fix
Sun, Sep 6
Fri, Sep 4
Aug 14 2020
Yeah, that sounds better, if that does not involve too much effort.
Aug 13 2020
I took a little more look at this and all of this cost model is very confusing. :( First I don't understand why the lists of intrinsics listed in switch-cases in getIntrinsicInstrCost and getTypeBasedIntrinsicInstrCost are different. fshr is listed only in getIntrinsicInstrCost but not in getTypeBasedIntrinsicInstrCost. It uses some more info than just types (like if some value is power of 2 or something), but even if we don't assume anything, it returns a far greater number (in this case 5) because it computes and accumulates cost for each of its argument while getTypeBasedIntrinsicInstrCost does not have a case handling for fshr so it just assumes it is cheap and returns 1.
Aug 12 2020
Not sure if it's worth it if there's no precedents. If people use our toolchain (clang or emscripten or rust or whatever) that wouldn't happen in the first place. Maybe it'd be sufficient to print some warning messages in CoalesceFeaturesAndStripAtomics if features don't match..?
Where are you planning to run that pass? Inlining runs within opt, before we can have any chances to run our own passes. You mean clang by 'frontend'? Can we run our own pass in clang?
Aug 11 2020
I think D79941 is supposed to work; what it did is move those logics into constructors of IntrinsicCostAttributes. For example, this constructor saves FMF and argument types, which are supposed to be used later in the same way. I think it'd be better to figure that out and fix it than restore the old code? Maybe the original author @samparker has some ideas on why :)
Aug 10 2020
Nice! Just wondering, then are we able to fold most of our SIMD intrinsics here too?
Aug 8 2020
+1 on only inlining into functions which have a superset of features; it looks safer that way.
Jul 30 2020
Jul 29 2020
It looks it was already addressed by https://github.com/llvm/llvm-project/commit/1603470e59a99a39ebdc4bf62a3a16c8c4ebea36.
Jul 28 2020
Jul 27 2020
Jul 24 2020
This looks fine to me, but I haven't worked on this part of code myself, so probably others who have can provide better review.
It looks like very smart transformation!
Jul 23 2020
Aha I see. Thanks.
Jul 22 2020
They seem to have been written that way because negative offsets aren't folded? What does it mean that they are duplicates of other patterns?
Jul 21 2020
Jul 16 2020
Jul 15 2020
Nit: NFC usually means code change that does not affect output, so adding tests or changing tests is not NFC by that definition :)
LGTM, but I posted a question on whether we need performBUILD_VECTORCombine in D83606.
Jul 14 2020
Wouldn't we want to add ISD::ABS to scalarizeSplatValue method in DAGCombiner.cpp too? WebAssembly wouldn't benefit from it, but it can benefit other targets that have a scalar abs instruction, and it is also consistent.
I think this is a nice idea! But I'd like people working on other targets to check this too.
Jul 13 2020
Jul 10 2020
Jul 9 2020
Jul 8 2020
Jul 6 2020
- Align FileCheck lines
Jun 19 2020
Jun 17 2020
Sorry for the delayed reply!
Jun 15 2020
Yeah I switched to this version to address your comment here. And yeah I think this is better that we don't violate assumptions on TEEs and keep ExplicitLocals to its core tasks, while we put all ugly things in fixUnwindMismatches, which is ugly anyway :$... Joking, but I think it's better that fixUnwindMismatches takes care of its own messes.
- comment fix
Closing in favor of D81851.
Jun 13 2020
(Phabricator tip: You can set parent / child revisions, which can be useful in case multiple CLs are in flight and have not been landed yet :) Probably you already know that, but anyway just in case)
Jun 12 2020
- is -> gets / becomes
Some lines in WebAssemblyInstrAtomics.td and WebAssemblyInstrSIMD.td look they exceed 80 cols, so please fix that too :)
- Fix typo in comment
Jun 11 2020
LGTM either with isNotDuplicable or isConvergent, as long as it works. I'm not familiar with what isConvergent is used for either, so..
Jun 10 2020
Jun 8 2020
Jun 7 2020
Question: Is this feature gonna be handled by -mtriple and not -mattrs like other features?
Nit: NFC usually means code change that does not affect output, so adding more tests is not NFC by that definition.
Jun 4 2020
I see. Thanks for the explanation.
Jun 3 2020
I'm not very familiar with how wasm debugger works, but do they use unreachable for breakpoints? Binaryen assumes unreachable is really unreachable and does all sorts of optimizations including DCE and everything, so if unreachable is actually used in debugger, that seems risky..?
Jun 1 2020
I didn't know about this optimization opportunity! Please make sure to run emscripten tests before landing because it contains some new assumptions.