grosbach (Jim Grosbach)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 10 2012, 12:58 PM (271 w, 4 d)

Recent Activity

Feb 13 2017

grosbach added a comment to D29770: [Assembler] Inline assembly diagnostics test..

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?).

Feb 13 2017, 9:00 AM

Jan 31 2017

grosbach added a comment to D29315: MCMacho: Allow __thread_ptr section after dwarf sections.

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

Jan 31 2017, 5:14 PM
grosbach added a comment to D29315: MCMacho: Allow __thread_ptr section after dwarf sections.

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?

Jan 31 2017, 1:20 PM
grosbach added inline comments to D29315: MCMacho: Allow __thread_ptr section after dwarf sections.
Jan 31 2017, 10:33 AM
grosbach added a comment to D29315: MCMacho: Allow __thread_ptr section after dwarf sections.

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 31 2017, 10:18 AM

Jan 19 2017

grosbach accepted D28861: [Assembler] Improve error when unable to evaluate expression..

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

Jan 19 2017, 11:20 AM

Jan 18 2017

grosbach added a comment to D28861: [Assembler] Improve error when unable to evaluate expression..

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?

Jan 18 2017, 10:19 AM

Dec 14 2016

grosbach added a comment to D27411: [Assembler] Better error messages for .org directive.

This is awesome! Thank you.

Dec 14 2016, 10:51 AM

Dec 12 2016

grosbach added inline comments to D27449: [ARM] Implement execute-only support in CodeGen.
Dec 12 2016, 2:41 PM

Dec 9 2016

grosbach requested changes to D17293: [MC] AsmLexer: add extensible identifier's character set support..
Dec 9 2016, 2:58 PM · Restricted Project

Dec 8 2016

grosbach added a comment to D27449: [ARM] Implement execute-only support in CodeGen.

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.

Dec 8 2016, 1:31 PM
grosbach added a comment to D27449: [ARM] Implement execute-only support in CodeGen.

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.

Dec 8 2016, 10:57 AM

Dec 1 2016

grosbach added a comment to D26214: [llvm] Implement support for -defsym assembler option.

Thank you!

Dec 1 2016, 5:36 PM
grosbach added inline comments to D26214: [llvm] Implement support for -defsym assembler option.
Dec 1 2016, 5:25 PM
grosbach requested changes to D26214: [llvm] Implement support for -defsym assembler option.

Please revert and fix.

Dec 1 2016, 11:11 AM
grosbach reopened D26214: [llvm] Implement support for -defsym assembler option.
Dec 1 2016, 11:09 AM

Jun 27 2016

grosbach added a comment to rL273814: Apply clang-tidy's modernize-loop-convert to lib/MC..

nice. thanks!

Jun 27 2016, 10:03 AM

Jun 17 2016

grosbach added a comment to D21471: [MCContext] Don't use getenv inside class constructor.

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

Jun 17 2016, 9:26 AM

Jan 8 2016

grosbach added inline comments to D16009: [lld]Non-MachO references shouldn't assert when checking for TLV..
Jan 8 2016, 6:07 PM · lld
grosbach retitled D16009: [lld]Non-MachO references shouldn't assert when checking for TLV. from to [lld]Non-MachO references shouldn't assert when checking for TLV..
Jan 8 2016, 12:43 PM · lld

Nov 17 2015

grosbach added a comment to D14717: [Assembler] Make fatal assembler errors non-fatal.

This is totally reasonable.

Nov 17 2015, 1:43 PM

Nov 16 2015

grosbach added a comment to D14646: [ARM,AArch64] Store source location of asm constant pool entries.

Very nice. Thank you!

Nov 16 2015, 9:33 AM

Nov 9 2015

grosbach added a comment to D14252: [AsmParser] Allow tokens to be put back in to the token stream..

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.

Nov 9 2015, 1:54 PM
grosbach added a comment to D14252: [AsmParser] Allow tokens to be put back in to the token stream..

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

Nov 9 2015, 10:05 AM
grosbach added a comment to rL249854: Clear SectionSymbols in MCContext::Reset.

Yep. Approved for 3.7.

Nov 9 2015, 10:02 AM · Restricted Project

Nov 3 2015

grosbach added a comment to rL251970: Don't create empty sections just to look like gas..

Nice. Thanks!

Nov 3 2015, 12:33 PM

Oct 26 2015

grosbach added inline comments to D14094: Add multikey_qsort to STLExtras.h.
Oct 26 2015, 3:24 PM
grosbach added inline comments to D14094: Add multikey_qsort to STLExtras.h.
Oct 26 2015, 3:12 PM
grosbach added a comment to D14094: Add multikey_qsort to STLExtras.h.

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.

Oct 26 2015, 2:43 PM
grosbach added a comment to D14053: Optimize StringTableBuilder.

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

Oct 26 2015, 1:21 PM

Oct 16 2015

grosbach committed rL250557: MC: Don't crash after issuing a diagnostic..
MC: Don't crash after issuing a diagnostic.
Oct 16 2015, 3:10 PM

Oct 14 2015

grosbach accepted D13188: [MachO] Stop generating *coal* sections..

LGTM. Thanks!

Oct 14 2015, 3:18 PM
grosbach added inline comments to D13188: [MachO] Stop generating *coal* sections..
Oct 14 2015, 1:05 PM

Oct 7 2015

grosbach added a comment to D13188: [MachO] Stop generating *coal* sections..

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.

Oct 7 2015, 3:43 PM
grosbach added a comment to D13188: [MachO] Stop generating *coal* sections..

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.

Oct 7 2015, 1:38 PM

Sep 29 2015

grosbach added a comment to D13011: [MC layer][AArch64] llvm-mc accepts 4-bit immediate values for "msr pan, #imm".

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 29 2015, 10:00 AM

Sep 23 2015

grosbach added a comment to D13104: Mips - Mark the section .eh_frame as writeable for pic.

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

Sep 23 2015, 10:35 AM
grosbach added a comment to D13104: Mips - Mark the section .eh_frame as writeable for pic.

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

Sep 23 2015, 9:43 AM

Sep 21 2015

grosbach accepted D13041: [llvm-readobj/MachO] Ensure we always have a valid reference to SegmentName.

LGTM

Sep 21 2015, 6:10 PM
grosbach added a comment to D13041: [llvm-readobj/MachO] Ensure we always have a valid reference to SegmentName.

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

Sep 21 2015, 5:45 PM
grosbach added a comment to D13011: [MC layer][AArch64] llvm-mc accepts 4-bit immediate values for "msr pan, #imm".

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

Sep 21 2015, 10:18 AM

Sep 10 2015

grosbach added a comment to D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option.

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.

Sep 10 2015, 2:34 PM
grosbach added a comment to D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option.

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.

Sep 10 2015, 8:33 AM

Sep 8 2015

grosbach added a comment to D10416: Use function attribute "arm-restrict-it".

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?

Sep 8 2015, 2:21 PM
grosbach added a comment to D10416: Use function attribute "arm-restrict-it".

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 8 2015, 11:31 AM

Sep 4 2015

grosbach added a comment to rL246604: [MC] Remove MCAssembler's copy of OS.

Nice. Thanks!

Sep 4 2015, 1:47 PM
grosbach added a comment to rL246840: [MC] Replace comparison with isUInt<32>..

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

Sep 4 2015, 10:52 AM

Sep 3 2015

grosbach requested changes to D11215: Adding altmacro support in integrated assembler. continue of D10591 .

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 3 2015, 11:08 AM

Sep 1 2015

grosbach added a comment to D12504: [llvm-readobj] MachO -- correctly dump section field 'Reserved3'.

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

Sep 1 2015, 10:59 AM

Aug 25 2015

grosbach added a comment to D12282: [MachO] Introduce MinVersion API.

Thanks!

Aug 25 2015, 11:30 AM
grosbach added a comment to D12282: [MachO] Introduce MinVersion API.

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

Aug 25 2015, 9:57 AM

Jun 23 2015

grosbach committed rL240432: Driver: Pass -I options to cc1as for .include search paths..
Driver: Pass -I options to cc1as for .include search paths.
Jun 23 2015, 11:25 AM

Jun 4 2015

grosbach committed rL239121: MC: Clean up naming in MCObjectFileInfo.h..
MC: Clean up naming in MCObjectFileInfo.h.
Jun 4 2015, 4:39 PM
grosbach committed rL239120: MC: Tidy up formatting and doc comments. NFC..
MC: Tidy up formatting and doc comments. NFC.
Jun 4 2015, 4:39 PM
grosbach committed rL239119: MC: Clean up the naming for MCMachObjectWriter. NFC..
MC: Clean up the naming for MCMachObjectWriter. NFC.
Jun 4 2015, 4:30 PM
grosbach committed rL239118: MC: Move MachObjectWriter::SectionAddress to private..
MC: Move MachObjectWriter::SectionAddress to private.
Jun 4 2015, 4:29 PM
grosbach committed rL239117: MC: Tidy up formatting and doc comments. NFC..
MC: Tidy up formatting and doc comments. NFC.
Jun 4 2015, 4:29 PM
grosbach committed rL239108: MC: Clean up naming in MCObjectWriter. NFC..
MC: Clean up naming in MCObjectWriter. NFC.
Jun 4 2015, 3:28 PM
grosbach committed rL239107: MC: Tidy up formatting and doc comments. NFC..
MC: Tidy up formatting and doc comments. NFC.
Jun 4 2015, 3:28 PM
grosbach committed rL239084: MC: Remove obsolete MachO UseAggressiveSymbolFolding..
MC: Remove obsolete MachO UseAggressiveSymbolFolding.
Jun 4 2015, 1:32 PM

Jun 1 2015

grosbach committed rL238800: MC: Tidy up LOH naming a bit. NFC..
MC: Tidy up LOH naming a bit. NFC.
Jun 1 2015, 4:59 PM
grosbach committed rL238799: MC: Tidy up formatting a bit. NFC..
MC: Tidy up formatting a bit. NFC.
Jun 1 2015, 4:59 PM

May 29 2015

grosbach committed rL238634: MC: Clean up MCExpr naming. NFC..
MC: Clean up MCExpr naming. NFC.
May 29 2015, 6:32 PM

May 18 2015

grosbach committed rL237594: MC: Clean up method names in MCContext..
MC: Clean up method names in MCContext.
May 18 2015, 11:47 AM
grosbach committed rL237595: MC: clang-format MCContext. NFC..
MC: clang-format MCContext. NFC.
May 18 2015, 11:47 AM

May 15 2015

grosbach committed rL237471: MC: MCCodeGenInfo naming update. NFC..
MC: MCCodeGenInfo naming update. NFC.
May 15 2015, 12:17 PM
grosbach committed rL237469: MC: Update MCCodeEmitter naming. NFC..
MC: Update MCCodeEmitter naming. NFC.
May 15 2015, 12:17 PM
grosbach committed rL237470: MC: clang-format. NFC..
MC: clang-format. NFC.
May 15 2015, 12:17 PM
grosbach committed rL237468: MC: Update MCFixup naming. NFC..
MC: Update MCFixup naming. NFC.
May 15 2015, 12:17 PM

May 13 2015

grosbach committed rL237275: MC: Modernize MCOperand API naming. NFC..
MC: Modernize MCOperand API naming. NFC.
May 13 2015, 11:41 AM

May 1 2015

grosbach committed rL236368: MC: Tidy up comments and clean up formatting a bit. NFC..
MC: Tidy up comments and clean up formatting a bit. NFC.
May 1 2015, 5:47 PM
grosbach committed rL236367: Fix spelling..
Fix spelling.
May 1 2015, 5:47 PM

Apr 2 2015

grosbach accepted D8798: MC: For variable symbols, maintain MCSymbol::Section as a cache..

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.

Apr 2 2015, 5:05 PM

Mar 24 2015

grosbach added a comment to D8217: Allowing MC backends to decide relaxation based on fixup resolution.

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.

Mar 24 2015, 4:38 PM
grosbach added a comment to D8586: MC: For variable symbols, maintain MCSymbol::Section as a cache..

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

Mar 24 2015, 3:43 PM

Mar 20 2015

grosbach added a comment to D8217: Allowing MC backends to decide relaxation based on fixup resolution.

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

Mar 20 2015, 11:49 AM
grosbach added a comment to D8217: Allowing MC backends to decide relaxation based on fixup resolution.

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?

Mar 20 2015, 10:56 AM
grosbach added a comment to D8217: Allowing MC backends to decide relaxation based on fixup resolution.

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 20 2015, 10:21 AM

Mar 12 2015

grosbach added a comment to D7775: Mangler: Don't prefix MS ABI names with a leading ? character.

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

Mar 12 2015, 5:11 PM

Feb 23 2015

grosbach accepted D7795: Only use IR intrinsincs for __builtin_longjmp / __builtin_setjmp when the target supports them..
Feb 23 2015, 12:18 PM
grosbach added a comment to D7795: Only use IR intrinsincs for __builtin_longjmp / __builtin_setjmp when the target supports them..

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

Feb 23 2015, 12:18 PM

Feb 19 2015

grosbach added a comment to D6383: [inline-asm] Fix scope of assembly macros.

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 19 2015, 1:50 PM

Feb 12 2015

grosbach added a comment to D7593: [MC] Use the non-EH register mapping in the debug_frame section..

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?

Feb 12 2015, 1:22 PM

Jan 26 2015

grosbach added a comment to D7181: Avoid combining adjacent stores at -O0 to improve debug experience.

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 26 2015, 3:09 PM

Jan 23 2015

grosbach added a comment to D7005: Add tail call optimization for thumb1-only targets rev. 3.

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.?

Jan 23 2015, 11:20 AM

Dec 27 2014

grosbach added a comment to D6185: Respect object format choice on Darwin.

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

Dec 27 2014, 2:57 PM

Dec 22 2014

grosbach committed rL224746: X86: Don't over-align combined loads..
X86: Don't over-align combined loads.
Dec 22 2014, 4:36 PM

Dec 17 2014

grosbach added a comment to D6672: Add parsing of 'foo@local"..

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

Dec 17 2014, 11:33 AM

Dec 15 2014

grosbach added a comment to D6670: [MC] Reset the MCInst in the matcher function before adding opcode/operands..

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

Dec 15 2014, 5:45 PM
grosbach added a comment to D6383: [inline-asm] Fix scope of assembly macros.

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."

Dec 15 2014, 3:38 PM

Nov 22 2014

grosbach added a comment to D6371: MC: Don't emit min version directives when -fno-integrated-as is on.

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 22 2014, 5:35 PM

Nov 21 2014

grosbach added a comment to D6354: Fix ARM's add->adr translations for expressions.

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

Nov 21 2014, 12:10 PM

Nov 7 2014

grosbach added a comment to D3161: SymbolRewriter: introduce the SymbolRewriter pass.

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

Nov 7 2014, 12:38 PM

Oct 21 2014

grosbach added a comment to D5681: X86_64 target now obeys llc -disable-red-zone option.

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

Oct 21 2014, 9:53 AM

Sep 2 2014

grosbach added a comment to D4960: Implement emitLeading/TrailingFence in the ARM backend.

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

Sep 2 2014, 4:00 PM

Aug 20 2014

grosbach added a comment to D4971: Improve Cost model for SLPVectorizer when we have a vector division by power of 2.

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

Aug 20 2014, 1:38 PM

Aug 13 2014

grosbach added a comment to D4129: [Reassociation] Add support for reassociation with unsafe algebra. .

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

Aug 13 2014, 10:17 AM

Aug 8 2014

grosbach added a comment to D4747: MC: Split the x86 asm matcher implementations by dialect.

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

Aug 8 2014, 1:31 PM

Jul 24 2014

grosbach accepted D4654: [x86] Make vector legalization of extloads work more like the "normal" vector operation legalization with support for custom target lowering and fallback to expand when it fails, and use this to implement sext and anyext load lowering for x86 in a....

LGTM, too.

Jul 24 2014, 2:56 PM
grosbach added a comment to D3507: Darwin vararg parameters support in assembler macros.

Please add Tim Northover and Kevin Enderby as reviewers.

Jul 24 2014, 9:45 AM