- User Since
- Oct 15 2012, 2:12 PM (309 w, 6 d)
Fri, Sep 21
Thu, Sep 20
Wed, Sep 19
Tue, Sep 18
Adding Steve here in case he wants to opine.
Sun, Sep 16
Fri, Sep 14
Thu, Sep 6
Wed, Sep 5
I'll get it tomorrow if no one does before.
Meta comment: please split the rename out. I don't mind the rename, but it makes the change much harder to review.
Tue, Sep 4
Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?
The change in name here from "line tables" to "directives only" feels a bit confusing. "Limited" seems to be a bit more clear, or even remaining line tables only. Can you explain where you were going with this particular set of changes in a bit more detail please?
Some inline comments.
Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.
Thu, Aug 30
LGTM, but let's get Stephen to ack as well.
Tue, Aug 28
Feel free to add any tests like this that test existing behavior without review in the future. If we want to change the behavior we should probably review that though :)
Yeah, that works for me. If we start getting something wrong it's an edge case we should probably document :)
Aug 24 2018
Going to guess you need someone to commit this for you.
Should we just have them mean the same thing and change it based on target?
Aug 22 2018
Oh, you probably want to fix the commit message here.
Aug 21 2018
Aug 7 2018
Aug 6 2018
LGTM. One inline comment.
Aug 5 2018
Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well.
Aug 1 2018
Jul 31 2018
Jul 30 2018
But you have other locations inside of functions so I don't get it.
Couple of comments.
Can you explain this a bit more with some examples of wrong relocations?
The patch is fine, in general, couple of comments:
Jul 25 2018
Bit of a hack, but I'm ok with it.
Jul 24 2018
Couple of small nits, but that's it. Still LGTM.
Jul 23 2018
Couple of small nits, ctopper's suggestion seems reasonable too. Only "better" way I had of dealing with this would be not folding the load at all in the first place, but it seems harder to justify given the existing unfolding machinery.
Jul 20 2018
Jul 19 2018
I don't necessarily think this should be part of DIBasicType since it'll be used almost never. Adding other reviewers to get thoughts on location.
Getting close, one inline comment.
LGTM. The comment is particularly helpful, thanks!
Jul 18 2018
It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?
Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.
Jul 17 2018
LGTM. Looks like a nice set of API simplifications.
I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.
You'll want to wait on Rui for an actual LGTM, but this looks good to me and will match the llvm support so we can try this out in more detail and see where we should move the proposals.
Checking out the archives for this list might come in handy:
LGTM. Not a fan of adding more stuff to TargetOptions, but for now it'll do.
LGTM. Thanks and sorry for the delay.
LGTM. Small organizational thoughts, but that's it.
Jul 16 2018
As a quick change this looks ok, some thoughts for cleanup:
Jul 12 2018
This is obviously good to start and to iterate on.
LGTM, a couple of inline requests. One small and one maybe less so.
Jul 11 2018
The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)
Jul 10 2018
Has ELFOSABI_HERMITCORE been officially allocated?