Eric is correct. These tests would be more suitable in llc. The clang tests should check that the inline asm is inserted into the IR in the correct form if there's any difference pre and post patch (I don't think there is?).

Aha! Yes, that was the piece I'd forgotten. Thank you.

In D29315#662038, @MatzeB wrote:

In D29315#661848, @grosbach wrote:

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

What's the motivation for this rule? Less work when striping a binary?

Also we do allow the __DATA/__nl_symbol_ptr section after the dwarf sections today it seems, shouldn't __DATA/__thread_ptr behave very similar?

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

Great! Incremental is definitely the way to go for this sort of thing. Thanks again.

This LGTM in general. I agree w/ Renato that CHECK-DAG is a bit unexpected. Can you elaborate on why that's better than plain CHECK?

This is awesome! Thank you.

In D27449#617442, @rengolin wrote:

In D27449#617363, @grosbach wrote:

Seems a reasonable thing to support in general. Were the concerns about supporting execute-only conceptually or about the design of this approach?

About the concept of unreadable code areas in general, and what benefits this really brings to security and performance of deeply embedded devices.

In D27449#617063, @rengolin wrote:

Right, I'm happy with the changes from the code side, but there were some concerns on IRC if we should support this at all.

Thank you!

Please revert and fix.

nice. thanks!

How is calling getenv() in a global static initializer any better? Seems a step backwards.

This is totally reasonable.

Very nice. Thank you!

Would it be reasonable to instead add target hooks for things like this? Either specialized things like the comment character handling or something more general like the ability to override recognizing an integer literal or whatever.

What are the alternatives? This is a pretty nasty hack.

Yep. Approved for 3.7.

Nice. Thanks!

Not 100% sure it's the best place, either; it's just what came to mind since the function we're replacing the invocation of is also there.

Cool. AFAIK, the only reason for the array_pod_sort() before was "seems good enough for now."

LGTM. Thanks!

If an assembly file explicitly references a section with the old name, we should not be re-mapping that to the new name. People writing assembly expect the output to be exactly what they put in the .s file and we should honor that.

The idea seems fine. The implementation is off, though. Why are we re-mapping the section names after the fact instead of setting them correctly in the first place? The call to setMachOCoalSections() is already even being added in exactly the same function which would need to be modified for that.

I don't see the text which describes the other values as UNPREDICTABLE. Can you provide a doc number and section reference? The v8.1 extension doc describes the other bits of the CRm field as being zero, but I don't see a mention of what happens if they're not. I believe you, I'm just wondering where I should be looking that I'm obviously not.

If you're arguing that the linker should not have to do this, then please make a case for that. You may well be right (I don't know ELF well enough to have an opinion on that).

So this is a compiler workaround for an MCLinker bug/restriction/missing feature? Why do we want to do that?

LGTM

Interesting. Would it be better to change the StringRef to a std::string instead? That way the data structure itself maintains the full info.

The MSR instruction is explicitly documented as taking a 4-bit immediate operand.

In D10414#243302, @jmolloy wrote:

In an ideal world, yes. However there's no guarantee that all ARM implementors will (a) be able to commit to LLVM or (b) use ToT. Perhaps they're building a project that uses clang, or a specific version of clang, and this tuning option makes things go faster on their architecture.

I'm not arguing about the defaults, just about the breakage of -mno-restrict-it.

In D10414#243056, @jmolloy wrote:

Non-restricted IT blocks are indeed deprecated for ARMv8 in the ARMARM. But there are circumstances where you may still want to emit them - the biggest example being you're compiling for a CPU microarchitecture that you *know* doesn't have a performance penalty on non-restricted IT blocks. Restricted IT blocks can pessimize code quite badly in some circumstances, and allowing people to turn it off for their target if needed is very important, IMO.

Bother, email response isn't showing up in Phab. Reposting here so there's a record. Sorry for the duplication on-list.

In D10416#241764, @ahatanak wrote:

In D10416#241642, @grosbach wrote:

For the backend attribute, having -arm-restrict-it (or equivalent) transitively disable HasV8Ops is really, really bad. Those should be completely independent selections. The former is about tuning instruction selection (via if-conversion in this case) based on the microarchitecture. The latter is about which instructions are available in the first place. The former does not imply the latter.

I think we want to discuss whether the latter (HasV8Ops) implies the former (restrict-it). If it makes no sense from the user's POV to not restrict IT blocks when targeting armv8, then we can conclude armv8 implies restrict-it and add FeatureRestrictedIT to the implied features list of HasV8Ops. If we do this, disabling restrict-it would transitively disable HasV8Ops. Does that make sense?

For the backend attribute, having -arm-restrict-it (or equivalent) transitively disable HasV8Ops is really, really bad. Those should be completely independent selections. The former is about tuning instruction selection (via if-conversion in this case) based on the microarchitecture. The latter is about which instructions are available in the first place. The former does not imply the latter.

Nice. Thanks!

Yikes. Nice catch. Are there any other similar instances we should make a similar fix for?

The linked documentation is very vague. There needs to be a lot more clarity on the design before this goes in. For example, what expressions are legal for %expr? It can't be anything, as an expression which requires a relocation would obviously not be reasonable. So what are the constraints? This introduces really nasty layering issues between the parser, lexer, and layout engines, to put it mildly. Before we can realistically talk about the code itself, those design issues need sorted out.

Pretty sure it's just to force the structure size to be a multiple of 64-bits.

Thanks!

Would it be better for these function to be in the header since they're trivial accessors?

This looks great and is exactly what I was looking for. Thank you! Now when I (or anyone else) come back looking through git history in some future year, we'll be able to figure out what was really going on here.

In D8217#144272, @colinl wrote:

I'm definitely open to cleaner implementations.

The issue with not doing anything in relaxInstruction() is relaxation will get stuck in an infinite loop. The relaxation loop assumes that relaxInstruction will eventually change fixupNeedsRelaxation or mayNeedRelaxation to false.

There's not enough information in the description for me to go on. Which edge cases? How/why were they wrong before?

Ah, right. I see what you're saying now.

Then why do you need this patch? If AsmBackend::fixupNeedsRelaxation() function is called, you know the fixup has not been resolved. Perhaps we just need to clarify the documentation of that interface to make that explicit?

I don't follow. From your description, it's still true that a fixup won't need relaxation if it's already been resolved. It's only if it hasn't that you need to ask more questions to decide what to do. That is exactly what information the code currently provides to the backend. That is, the AsmBacked::fixupNeedsRelaxation() is only called if the fixup was not resolved. You can still decide how much relaxation to do (or not do) however you want.

Conceptually, it's a bit odd to me that something for windows changes macho output all, but I can see why it happens here.

So the builtins get codegen'ed to the library functions if the backend doesn't support the intrinsics? Seems reasonable.

I'm not particularly in favor of the approach taken in this patch, where only module assembly can define macros which get used in functions. It's not very intuitive.

Honestly, this is all really horrible. I'm pretty content with the status quo. Maybe we can agree to not implement this? Users can this functionality with the C preprocessor.

Oof. The patch itself seems reasonable. I tend to agree with your thought that it may not be a root cause. Do you have any ideas about what a deeper solution would look like?

There should be exactly zero DAG combines that have to run, ever. They are by definition not required for correctness. Anything else is a bug, and a bad one.

In D7005#110256, @Langohr wrote:

When re-analyzing prologue and epilogue generation code for thumb1, it seems to me that there is another issue related to the fix done in r210889. The code in head still refers to DPR registers within a possibly available VFP unit and corresponding register spilling and restoring. For thumb1, DPR registers are, however, inaccessible. Prologue and epilogue, thus, should only consider AAPCS calling convention and not AAPCS-VFP. I doubt that this part of the code had shown up as part of copy-and-paste from thumb2/ARM code.?

Glad to have a look. On holiday at the moment so if I don't get to it before then, please ping me on the 4th when I'm back at my desk where I have all the relevant bits available.
Jim

Do you want the upper-case variant as well? i.e., foo@LOCAL? That would match the other entries in the switch.

Clearing the MCInst seems reasonable. That appears to be the expected behaviour, effectively.

Use of assembler directives at all in inline assembly has always been an edge case that only sort-of works and is sketchily supported. The canonical answer to anyone having problems with them is "don't do that."

The non integrated assembler can't handle lots of things the compiler emits on Darwin. The only realistic use of that command line option now is when processing .s files.

Seems reasonable to me, but Tim will definitely have a better eye for the details than I.

Looking much improved. A few questions, but in general I think this is fine.

Does this mean the clang -m[no-]red-zone option wasn't hooked up either? Does this fix that if so?

I should defer to Tim Northover on this sort of thing. He's better equipped than I to get into the details of atomics.

Very interesting. Not just X86, either. Filed http://llvm.org/bugs/show_bug.cgi?id=20714 for the AArch64 equivalent.

I'm fine with that if Chandler is. It all seems reasonable to me, but I'm not a floating point expert.

I believe Daniel's right that WasOriginallyInvalidOperand is unused and can be nuked in MatchAndEmitIntelInstruction().

LGTM, too.

Please add Tim Northover and Kevin Enderby as reviewers.