Jakob Stoklund Olesen
2012-Aug-21 21:02 UTC
[LLVMdev] Let's get rid of neverHasSideEffects
All, TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects. It's possible to override this behavior by setting neverHasSideEffects = 1. It was originally the intention that most instructions have patterns, but that's not the way it worked out. It is often more convenient to use def : Pat<>, and sometimes custom instruction selection is required. As a result, many instructions are defined without a pattern, and we often forget to set neverHasSideEffects = 1. $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc lib/Target/ARM/ARMGenInstrInfo.inc:727 lib/Target/X86/X86GenInstrInfo.inc:920 I don't think more than half of those UnmodeledSideEffects flags should be there. I want to stop inferring instruction properties from patterns in TableGen. It has become very hard to read instruction definitions when some properties are inferred, but only half the time. We can still use any patterns to emit TableGen warnings. If an instruction has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should complain. I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. Any suggestions for a clean transition? I was thinking about first adding a -Winfer flag that warns whenever TableGen infers a property that wasn't already set in the .td file. /jakob
On Tue, Aug 21, 2012 at 2:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk>wrote:> All, > > TableGen likes to infer the MCID::UnmodeledSideEffects flag from an > instruction's pattern. When an instruction doesn't have a pattern, it is > assumed to have side effects. > > It's possible to override this behavior by setting neverHasSideEffects = 1. > > It was originally the intention that most instructions have patterns, but > that's not the way it worked out. It is often more convenient to use def : > Pat<>, and sometimes custom instruction selection is required. > > As a result, many instructions are defined without a pattern, and we often > forget to set neverHasSideEffects = 1. > > $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc > lib/Target/ARM/ARMGenInstrInfo.inc:727 > lib/Target/X86/X86GenInstrInfo.inc:920 > > I don't think more than half of those UnmodeledSideEffects flags should be > there. > > > I want to stop inferring instruction properties from patterns in TableGen. > It has become very hard to read instruction definitions when some > properties are inferred, but only half the time. > > We can still use any patterns to emit TableGen warnings. If an instruction > has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should > complain. >For what little it's worth (as I rarely touch the target td files), I like this plan. =] I just wanted to comment on the migration bit:> I can't just kill off the neverHasSideEffects flag, that could cause > miscompilations by clearing the UnmodeledSideEffects bit on instructions > that should have it. >Why not mass-update all the td files in the tree to explicitly set the flag exactly as it is being inferred today, then kill the inference. The behavior is the same, but now explicit, and we can go through and remove all the places that make no sense. This email (or a more clearly announce-y email) can update folks w/ out-of-tree targets. Add the warning in the same go so that people immediately get complaints from tablegen until they update. Add a big red note to the release notes for those with out-of-tree targets that don't track trunk. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120821/333aecd6/attachment.html>
I'd personally like to see a moratorium on changes to tablegen until someone writes a reference manual for it that covers the application specific backends for code generation. On 08/21/2012 02:02 PM, Jakob Stoklund Olesen wrote:> All, > > TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects. > > It's possible to override this behavior by setting neverHasSideEffects = 1. > > It was originally the intention that most instructions have patterns, but that's not the way it worked out. It is often more convenient to use def : Pat<>, and sometimes custom instruction selection is required. > > As a result, many instructions are defined without a pattern, and we often forget to set neverHasSideEffects = 1. > > $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc > lib/Target/ARM/ARMGenInstrInfo.inc:727 > lib/Target/X86/X86GenInstrInfo.inc:920 > > I don't think more than half of those UnmodeledSideEffects flags should be there. > > > I want to stop inferring instruction properties from patterns in TableGen. It has become very hard to read instruction definitions when some properties are inferred, but only half the time. > > We can still use any patterns to emit TableGen warnings. If an instruction has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should complain. > > I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. > > Any suggestions for a clean transition? I was thinking about first adding a -Winfer flag that warns whenever TableGen infers a property that wasn't already set in the .td file. > > /jakob > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Aug 21, 2012, at 2:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:> All, > > TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects. > > It's possible to override this behavior by setting neverHasSideEffects = 1. > > It was originally the intention that most instructions have patterns, but that's not the way it worked out. It is often more convenient to use def : Pat<>, and sometimes custom instruction selection is required. > > As a result, many instructions are defined without a pattern, and we often forget to set neverHasSideEffects = 1. > > $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc > lib/Target/ARM/ARMGenInstrInfo.inc:727 > lib/Target/X86/X86GenInstrInfo.inc:920 > > I don't think more than half of those UnmodeledSideEffects flags should be there. > > > I want to stop inferring instruction properties from patterns in TableGen. It has become very hard to read instruction definitions when some properties are inferred, but only half the time.Yes, please. This does get a bit more complicated for properties like mayLoad, mayStore and such, but I still like requiring them to be explicit, personally. When there's a pattern on the instruction, we can check for semantic mismatches and throw an error if we find one.> > We can still use any patterns to emit TableGen warnings. If an instruction has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should complain. > > I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. > > Any suggestions for a clean transition? I was thinking about first adding a -Winfer flag that warns whenever TableGen infers a property that wasn't already set in the .td file. >I like that. Possibly with the addition that we can filter by a specific property. -Winfer=neverHasSideEffects, e.g., would only show when that specific property is inferred. Beyond that, I don't see an alternative to an audit of the instructions that get flagged by such a warning. :( -Jim> /jakob > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Aug 21, 2012, at 2:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:> All, > > TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects.Hi Jakob, I don't understand what you're saying. Are you proposing that all properties (may load, store, side effects) be explicitly added to all instructions, and the pattern only be used to produce warnings? Won't this hugely bloat the .td files? -Chris> > It's possible to override this behavior by setting neverHasSideEffects = 1. > > It was originally the intention that most instructions have patterns, but that's not the way it worked out. It is often more convenient to use def : Pat<>, and sometimes custom instruction selection is required. > > As a result, many instructions are defined without a pattern, and we often forget to set neverHasSideEffects = 1. > > $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc > lib/Target/ARM/ARMGenInstrInfo.inc:727 > lib/Target/X86/X86GenInstrInfo.inc:920 > > I don't think more than half of those UnmodeledSideEffects flags should be there. > > > I want to stop inferring instruction properties from patterns in TableGen. It has become very hard to read instruction definitions when some properties are inferred, but only half the time. > > We can still use any patterns to emit TableGen warnings. If an instruction has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should complain. > > I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. > > Any suggestions for a clean transition? I was thinking about first adding a -Winfer flag that warns whenever TableGen infers a property that wasn't already set in the .td file. > > /jakob > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On 08/21/2012 03:02 PM, Chris Lattner wrote:> On Aug 21, 2012, at 2:02 PM, Jakob Stoklund Olesen<stoklund at 2pi.dk> wrote: > >> All, >> >> TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects. > Hi Jakob, > > I don't understand what you're saying. Are you proposing that all properties (may load, store, side effects) be explicitly added to all instructions, and the pattern only be used to produce warnings? > > Won't this hugely bloat the .td files? > > -Chris >For mips16, I have NO patterns in instructions. The instructions all stand by themselves independent of how they are used in code generation. The patterns are all separated. I think this is better from the point of view of data abstraction and information hiding. I don't want to know about the details of instruction layout when I'm thinking about the higher level use of the instruction.>> It's possible to override this behavior by setting neverHasSideEffects = 1. >> >> It was originally the intention that most instructions have patterns, but that's not the way it worked out. It is often more convenient to use def : Pat<>, and sometimes custom instruction selection is required. >> >> As a result, many instructions are defined without a pattern, and we often forget to set neverHasSideEffects = 1. >> >> $ grep -c UnmodeledSideEffects lib/Target/*/*GenInstrInfo.inc >> lib/Target/ARM/ARMGenInstrInfo.inc:727 >> lib/Target/X86/X86GenInstrInfo.inc:920 >> >> I don't think more than half of those UnmodeledSideEffects flags should be there. >> >> >> I want to stop inferring instruction properties from patterns in TableGen. It has become very hard to read instruction definitions when some properties are inferred, but only half the time. >> >> We can still use any patterns to emit TableGen warnings. If an instruction has a sideeffecting pattern, but hasSideEffects isn't set, TableGen should complain. >> >> I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. >> >> Any suggestions for a clean transition? I was thinking about first adding a -Winfer flag that warns whenever TableGen infers a property that wasn't already set in the .td file. >> >> /jakob >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Jakob Stoklund Olesen
2012-Aug-21 22:45 UTC
[LLVMdev] Let's get rid of neverHasSideEffects
On Aug 21, 2012, at 3:02 PM, Chris Lattner <clattner at apple.com> wrote:> > On Aug 21, 2012, at 2:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote: > >> All, >> >> TableGen likes to infer the MCID::UnmodeledSideEffects flag from an instruction's pattern. When an instruction doesn't have a pattern, it is assumed to have side effects. > > Hi Jakob, > > I don't understand what you're saying. Are you proposing that all properties (may load, store, side effects) be explicitly added to all instructions, and the pattern only be used to produce warnings?Yes. The side effect inference is worse than the load/store inference, but features like this work best when they work all the time. If all instructions had patterns, and we could accurately infer properties, it wouldn't be a problem.> Won't this hugely bloat the .td files?Not really, TableGen has a fairly convenient syntax for bulk flagging: let mayLoad = 1 in { let Defs = [AL,EFLAGS,AX], Uses = [AX] in def DIV8m : I<0xF6, MRM6m, (outs), (ins i8mem:$src), // AX/[mem8] = AL,AH "div{b}\t$src", [], IIC_DIV8_MEM>; let Defs = [AX,DX,EFLAGS], Uses = [AX,DX] in def DIV16m : I<0xF7, MRM6m, (outs), (ins i16mem:$src), // DX:AX/[mem16] = AX,DX "div{w}\t$src", [], IIC_DIV16>, OpSize; let Defs = [EAX,EDX,EFLAGS], Uses = [EAX,EDX] in // EDX:EAX/[mem32] = EAX,EDX def DIV32m : I<0xF7, MRM6m, (outs), (ins i32mem:$src), "div{l}\t$src", [], IIC_DIV32>; // RDX:RAX/[mem64] = RAX,RDX let Defs = [RAX,RDX,EFLAGS], Uses = [RAX,RDX] in def DIV64m : RI<0xF7, MRM6m, (outs), (ins i64mem:$src), "div{q}\t$src", [], IIC_DIV64>; } It is a complete coincidence that these instructions happen to be missing neverHasSideEffects = 1 ;) Often, you can set the mayLoad flag as part of a multiclass when you define the *rm variant. X86 is the worst case because so many instructions can fold loads and stores. You can handle most of them by setting the mayLoad and mayStore flags in the big ArithBinOp multiclasses. You can see the problem in the big ArithBinOp_RF multiclass in X86InstrArithmetic.td. It's not obvious at a glance which of those instructions have patterns, and as a result we forgot to set neverHasSideEffects on the *_REV and *8i8 variants. I don't think it would detract to add "let mayLoad = 1 in {…}" around the *rm groups etc. Or you could set the flags on the various BinOpRMW super classes. /jakob
Jakob Stoklund Olesen
2012-Aug-21 23:03 UTC
[LLVMdev] Let's get rid of neverHasSideEffects
On Aug 21, 2012, at 2:10 PM, Chandler Carruth <chandlerc at google.com> wrote:> I just wanted to comment on the migration bit: > > I can't just kill off the neverHasSideEffects flag, that could cause miscompilations by clearing the UnmodeledSideEffects bit on instructions that should have it. > > Why not mass-update all the td files in the tree to explicitly set the flag exactly as it is being inferred today, then kill the inference. The behavior is the same, but now explicit, and we can go through and remove all the places that make no sense.This would work for the mayLoad / mayStore flags. You'll get a bunch of warnings on your loads and stores. For hasSideEffects, I will be changing what happens when you do nothing (i.e., set neither neverHasSideEffects nor hasSideEffects). We need a tool that can point out those instructions so they can be audited, as Jim suggested. I think we should make sure that such a tool is available at least in the the next release. /jakob -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120821/9a0fcfb2/attachment.html>
On 08/21/12 16:49, Jim Grosbach wrote:> > I like that. Possibly with the addition that we can filter by a specific property. -Winfer=neverHasSideEffects, e.g., would only show when that specific property is inferred. > > Beyond that, I don't see an alternative to an audit of the instructions that get flagged by such a warning. :(This proposal would certainly make my life easier by having an easier to maintain insn table. For instance, we had to create two store insn classes: one which defined mayStore and one that didn't. The former was to be used when an insn had no pattern and the latter, when mayStore was implied in the pattern. If only TableGen wouldn't warn about non-conflicting attributes at least... -- Evandro Menezes Austin, TX emenezes at codeaurora.org Qualcomm Innovation Center, Inc is a member of the Code Aurora Forum