- User Since
- Oct 15 2012, 2:12 PM (389 w, 1 d)
Fri, Mar 27
Sure. I'd prefer we remove it from the struct completely, but that can happen later.
Mon, Mar 23
Fri, Mar 20
Tue, Mar 17
In general I think a lot of this looks pretty good. It could use some more comments - in particular to call out what a lot of this code is used for. It's not used on a typical linking path and so could be confusing to people going through the code.
Mon, Mar 16
Fri, Mar 13
Fix comments around full unroller.
Fixed the clang test.
Couple of random comments, but feel free to fix and commit. I think anything else can be handled post-review.
Thu, Mar 12
So I'm still concerned that this is on TargetOptions at all. Ideally for new attributes like this we'd only put them on the IR - at the way you've got it we're making resetTargetOptions even harder to remove. I don't think this is the right path forward (as removing resetTargetOptions is the right path) and this is only making that harder.
Wed, Mar 11
Tue, Mar 10
Mon, Mar 9
To be clear I think this is close to being acceptable, I'm just asking some questions before hitting the ack and getting things tidied up a bit so the next person doesn't have to ask :)
The change looks good to me regardless, but i'd like to understand the issue causing the variability in behaviour before giving formal approval, as non-determinism like this is bad, and might indicate something like an unitialised variable somewhere or some other bug.
I will also be nervous to approve this change, without understanding how the test fails.
Also the commit message is awesome, but would be good to get the commit message represented as comments in lots of the final code if possible :)
This is wanting a command line flag rather than setting it on the function, or rather in addition to being able to set it on the function? Also "this" in the commit message isn't quite clear. Having this be a separate set of both global function attributes and different than the other fp-math attributes means it really should be called out fairly explicitly why they need to be different, what the semantics are, etc.
Couple of inline comments. I'm trying pretty hard to be able to get rid of TargetOptions.h some day if possible. Any thoughts on ways to do this without?
Mon, Mar 2
Feb 27 2020
Can you document the semantics somewhere? (I have no idea if other patches have done anything like this, but it does seem like something that should be documented somewhere).
Most of the other tools are llvm- any reason for the change?
Feb 25 2020
This probably qualifies as "obvious" :)
Feb 24 2020
I think this is OK. Would you mind splitting out the undefined change from the register name change when committing?
Feb 19 2020
Fair enough. The DWARF committee has a different set of write-up requirements. Let me, aprantl, probinson, or others know if you have issues.
Some nits and style/comment/readability requests. Generally pretty happy with how it looks though. And thanks James for the numerous rounds of reviews already! :)
Feb 18 2020
Feb 13 2020
Feb 10 2020
This all sounds good to me (other than "hey can we start getting rid of these intrinsics?" :)
Jan 27 2020
Do we need a full separate pass structure for mlir or is there some sharing we can do here as this (at the IR level) is encompassed by GlobalDCE.
Jan 22 2020
Still looks good to me.
Jan 16 2020
Jan 15 2020
It'd be much nicer to get rid of resetTargetOptions here? Can we do that yet?
Jan 9 2020
Jan 8 2020
Sadly I'm just noticing this:
Jan 7 2020
Dec 27 2019
Looks ok to me.
Dec 26 2019
Ping ping goes the trolley.
Dec 19 2019
Dec 18 2019
Formatting and parens changes.
Dec 17 2019
Dec 13 2019
Fix for some slightly better API uses.
Dec 12 2019
Abandoning this one in favor of maskray's revision.
Dec 11 2019
clang-format changed lines.
The proposal seems interesting, but needs some more review.
Fan of this change, but let's definitely wait for more reviews :)
Dec 10 2019
Dead code is dead code :) LGTM.