Sure. I'd prefer we remove it from the struct completely, but that can happen later.

In D68065#1937331, @tmsriram wrote:

In D68065#1934866, @MaskRay wrote:

I am very glad to see that we have made progress by landing D68063 (llvm/CodeGen/CommandFlags.inc) and D73674 (llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp). Basic block sections is agreed to be useful even outside Properller.

There are several optimizations goals:

• Alignment inserting
• Automatic cache prefetching
• Large code model addressing can lower performance quite a bit. A post-link scheme can relax large code model addressting to small code model addressing.
• ...
  
  There is a CPU erratum that we want to mitigate.
• Intel's Jump Condition Code Erratum
  
  By making this change, we will go the object file level route: annotate object files with metadata so that certain transformations can be performed.
  
  Whether this scheme can satisfy the goals and avoid the erratum, and the uncertainty about how many stuff we will have to reinvent is my biggest concerns.
  
  On https://lists.llvm.org/pipermail/llvm-dev/2020-February/139543.html (my brainstorming), I mentioned we may achieve our goals and make it suitable for future optimizations by using a file format with more semantics (rather than an object file). I hope we can think more on this, rather than rush to conclusions "this is redoing full LTO. it can't scale"
  
  Considering the above points, I re-iterate my "Request Changes". We need a plan to prove that we can achieve our optimization goals while avoiding caveats (erratum).

@echristo @ruiu

If the JCC erratum is the only concern then we are able to show now with experiments that Propeller can produce JCC erratum free binaries with almost no performance impact and only by using the existing assembler mitigations : http://lists.llvm.org/pipermail/llvm-dev/2020-March/140134.html

Let's use that thread to continue to investigate how the linker could potentially do a better job of handling this or other erratums in general. Could we please unblock this?

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.

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.

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.

Hi Louis,

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?

In D75836#1912574, @aprantl wrote:

Doing it this way should be fine, too.

LGTM.

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?

This probably qualifies as "obvious" :)

I think this is OK. Would you mind splitting out the undefined change from the register name change when committing?

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! :)

In D70172#1879481, @jpienaar wrote:

This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build with assertions on (clean build at e8e078c just before this change, broken at this, assert triggering at build fix commit).

https://buildkite.com/mlir/mlir-core/builds/2792#a54fb239-718b-4f0b-a309-f83e46ceb252

This all sounds good to me (other than "hey can we start getting rid of these intrinsics?" :)

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.

Still looks good to me.

OK.

It'd be much nicer to get rid of resetTargetOptions here? Can we do that yet?

Sadly I'm just noticing this:

Looks ok to me.

Ping ping goes the trolley.

Formatting and parens changes.

LGTM.

Fix for some slightly better API uses.

In D71395#1781896, @mclow.lists wrote:

It appears that we have multiple things in review here.
See also D71096.
Also, is this in response to https://bugs.llvm.org/show_bug.cgi?id=44145 ? (which was closed as 'works for me')

Abandoning this one in favor of maskray's revision.

Reopening this.

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 :)

Dead code is dead code :) LGTM.