- User Since
- Jul 10 2012, 12:58 PM (431 w, 6 d)
Feb 13 2017
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?).
Jan 31 2017
Aha! Yes, that was the piece I'd forgotten. Thank you.
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.
Jan 19 2017
Great! Incremental is definitely the way to go for this sort of thing. Thanks again.
Jan 18 2017
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?
Dec 14 2016
This is awesome! Thank you.
Dec 12 2016
Dec 9 2016
Dec 8 2016
Dec 1 2016
Please revert and fix.
Jun 27 2016
Jun 17 2016
How is calling getenv() in a global static initializer any better? Seems a step backwards.
Jan 8 2016
Nov 17 2015
This is totally reasonable.
Nov 16 2015
Very nice. Thank you!
Nov 9 2015
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.
Nov 3 2015
Oct 26 2015
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."
Oct 16 2015
Oct 14 2015
Oct 7 2015
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.
Sep 29 2015
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.
Sep 23 2015
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?
Sep 21 2015
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.
Sep 10 2015
Bother, email response isn't showing up in Phab. Reposting here so there's a record. Sorry for the duplication on-list.
Sep 8 2015
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.
Sep 4 2015
Yikes. Nice catch. Are there any other similar instances we should make a similar fix for?
Sep 3 2015
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.
Sep 1 2015
Pretty sure it's just to force the structure size to be a multiple of 64-bits.
Aug 25 2015
Would it be better for these function to be in the header since they're trivial accessors?
Jun 23 2015
Jun 4 2015
Jun 1 2015
May 29 2015
May 18 2015
May 15 2015
May 13 2015
May 1 2015
Apr 2 2015
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.
Mar 24 2015
There's not enough information in the description for me to go on. Which edge cases? How/why were they wrong before?
Mar 20 2015
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.
Mar 12 2015
Conceptually, it's a bit odd to me that something for windows changes macho output all, but I can see why it happens here.
Feb 23 2015
So the builtins get codegen'ed to the library functions if the backend doesn't support the intrinsics? Seems reasonable.
Feb 19 2015
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.
Feb 12 2015
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?
Jan 26 2015
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.
Jan 23 2015
Dec 27 2014
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.
Dec 22 2014
Dec 17 2014
Do you want the upper-case variant as well? i.e., foo@LOCAL? That would match the other entries in the switch.
Dec 15 2014
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."
Nov 22 2014
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.
Nov 21 2014
Seems reasonable to me, but Tim will definitely have a better eye for the details than I.
Nov 7 2014
Looking much improved. A few questions, but in general I think this is fine.
Oct 21 2014
Does this mean the clang -m[no-]red-zone option wasn't hooked up either? Does this fix that if so?
Sep 2 2014
I should defer to Tim Northover on this sort of thing. He's better equipped than I to get into the details of atomics.
Aug 20 2014
Very interesting. Not just X86, either. Filed http://llvm.org/bugs/show_bug.cgi?id=20714 for the AArch64 equivalent.
Aug 13 2014
I'm fine with that if Chandler is. It all seems reasonable to me, but I'm not a floating point expert.
Aug 8 2014
I believe Daniel's right that WasOriginallyInvalidOperand is unused and can be nuked in MatchAndEmitIntelInstruction().
Jul 24 2014
Please add Tim Northover and Kevin Enderby as reviewers.