Details
Diff Detail
Event Timeline
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
73 | This is not your bug, but still worth mentioning. This pass preserves globalsAA, doesn't depend on it. In other words, this can go away. | |
537 | I'd remove the FIXME. No other pass mentions it, and I'm not sure if there's active work on this. | |
541 | I don't think there's a bug tracker for this (probably there should be one tho?). |
Thanks, David and Davide.
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
73 | Right, I noticed that too, but forgot about it. I'll remove it. | |
537 | You're probably right. | |
541 | I don't know about a bug tracker, but there are FIXMEs for this in many ported passes. I think it's good to have these fixmes regardless of what exactly happens in the future, simply to serve as (another) marker for passes where it makes *some* sense. |
This one looks good modulo two small comments.
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
526 | This function can be inlined (line 60), no? | |
541 | Yes a comment here makes sense, thanks! Can you please copy'n'paste from any of the passes I already ported (so that we have the same comment everywhere and it's easier to grep and/or locate when we want to replace) |
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
526 | Sure, I don't feel strongly about it either way. Will do. | |
541 | Unfortunately, it's already very non-uniform. We have at least: $ find . -name *.cpp | xargs cat | grep // | grep -i preserve | grep CFG // FIXME: Need a way to preserve CFG analyses here! // FIXME: There is no setPreservesCFG in the new PM. When that becomes // FIXME: This pass should preserve the CFG. // FIXME: BDCE should also 'preserve the CFG'. //FIXME: setPreservesCFG is not currently supported in the new PM. // FIXME: ADCE should also 'preserve the CFG'. // FIXME: once we have an equivalent of AU.setPreservesCFG() in the // FIXME: This pass should also 'preserve the CFG'. // FIXME: Reassociate should also 'preserve the CFG'. I can try to find all of those - I'm sure there are some this grep missed - and replace with the same string (as a separate patch, of course), but there'd be no way to enforce it for future ports. |
LGTM
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
541 | I'm fine with the dummy implementation but I'd like to hear what David/Chandler/Sean have to say about it. Maybe you can just leave the comment as his and defer that to a different patch (maybe start a thread on llvm-dev)? How does that sound? |
lib/Transforms/Scalar/Float2Int.cpp | ||
---|---|---|
541 | SGTM. |
This is not your bug, but still worth mentioning. This pass preserves globalsAA, doesn't depend on it. In other words, this can go away.