- User Since
- Mar 16 2020, 1:52 PM (151 w, 1 d)
Nov 29 2022
Mar 17 2022
Dec 14 2021
Aug 2 2021
Jul 28 2021
Happy to welcome Tim, agree with @emaste 's comment
Many thanks for getting this spelled out.
May 11 2021
This puts the "how to report" instructions front and center, which is by far the most important thing in the doc. Thank you!
Apr 21 2021
Mar 24 2021
Mar 23 2021
I want to avoid being the first one to approve, given our mutual affiliation, but at least wanted to confirm the story: @george.burgess.iv will be a valuable contact given his existing LLVM affiliation and deep involvement with the deployment of LLVM toolchains across several Google products.
Oct 19 2020
Accepted and ready to land. Could I impose on someone with commit access? Thanks.
Oct 16 2020
<note to self: procedure is "five business days", so next action here is Monday October 17>
Oct 9 2020
Thanks for writing this down. I think this presents a consistent state on our way to something more permanent. LGTM.
Oct 6 2020
Blast from the past. It seems like there's still benefit in checking in this documentation, appropriately updated to reflect the fact that SESES is now actually in the tree.
Aug 27 2020
And now his watch is ended. Thank you for getting this started!
Jul 27 2020
Jul 24 2020
Jul 23 2020
Glad that we could make this simpler without making the resulting code much slower!
Jun 10 2020
Ack, thanks for setting me straight on that. Yes, I was talking about the mapping of assembly instructions back to sourceloc that could be displayed in GDB.
LGTM, one question about source locations
Jun 9 2020
Thank you for the analysis and for the example, they were really helpful. Agree the approaches are different. They feel similar enough I wish we could find one that satisfied all relevant requirements, but I can't say with any certainty that e.g. SESES' performance is on par with the mitigation proposed here.
Jun 5 2020
Isn't this pass basically SESES? https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
May 20 2020
Took a quick look and seems sane -- will look after Craig's comment is addressed and build is passing
May 18 2020
Of course that raises a fun question: where do we put the code to make sure we warn if the pass is disabled? It can't be in the pass itself.
May 13 2020
Not sure we want to move this into the Parser -- SLH is a property of code generation, and I think it's possible (through LTO?) to mismatch the flags between parse and codegen.
May 12 2020
I'm no expert in the pass manager, but given the very targeted applicability of LVI it definitely seems like our goal should be 0% impact for the vast majority of compilations that don't concern themselves with it.
May 11 2020
May 8 2020
The extra comments and the new variable name are all helpful. Thanks again.
May 7 2020
I mean, yes, sure, I want that too, but that doc just says the Intel patches to GNU binutils work by "by inserting an LFENCE instruction after each instruction that performs a load". And the binutils implementation does seem to be trying to make that true?
Calling special attention to the comment at line 341, since I think it affects the correctness of the pass.
Apr 27 2020
Apr 23 2020
Still some comments to address, but I think this is substantially close to where it needs to be. Thanks so much for your work making the code more straightforward and readable!
Whew. I still think putting this in the X86 assembly parser (so it doesn't round-trip) is crazypants, and I wish we were relying more on instruction metadata rather than hardcoded lists, but if I take those decisions as given then this implementation seems fine.
Apr 15 2020
Reviewed the algorithm, have to confess I skipped the tests.
Apr 14 2020
Apr 13 2020
Accepting modulo some comments to be addressed. Most of my review effort was spent on the data structure and algorithm employed as well as code style and readability.
Apr 10 2020
I accidentally spent time reviewing this, only to cross-reference something in the LLVM code and find another diff (https://reviews.llvm.org/D76812) was written to answer Zola's request and has already been submitted.
Apr 9 2020
Apr 7 2020
I'll wait for current comments to be addressed before doing my next round here.
Some more comments. FWIW, I'm doing rounds of review as I can in some evening quiet or during my son's nap. This is a huge change and it's really hard to get any part of it into my head at once in a reasonable amount of time.
Apr 4 2020
Apr 3 2020
Adding a few early style notes for the next round, but overall echo @chandlerc that this seems significantly outside of normal LLVM code.
Mar 18 2020
First, want to apologize for the high latency -- thanks to (gestures vaguely in the air) I've been mostly offline during the day and trying to catch up at night.
Mar 16 2020
This seems like a much more robust approach for mitigating inline asm. Thanks for that!