> On 2011-09-21 07:50:10, Steve Reinhardt wrote: > > Were there places where the ambiguity between a structure field access and > > the type notation were giving you problems? Would they have been fixed > > just by making the one change here where the regex matches only valid type > > extensions and not any sequence of characters after a dot? > > > > My reluctance here is that you're substituting one ambiguous overloaded > > syntax for another... it's unlikely that I have an identifier that by > > itself ends in "_uq" but it seems equally unlikely that I've got a > > structure field named "uq". Between the two, IMO, the dot syntax is > > slightly more meaningful since the type qualifiers are a little bit like > > union fields. > > > > It's fine to me to change it, but it seems like we should change it to > > something that's clearly less ambiguous; how about colon? There's some > > potential ambiguity with ?:, but that's not a very common operator, and > > even when there's potential ambiguity it would be easy to fix by requiring > > spaces around the : when you don't want it to be treated as a type > > qualifier. > > > > Some other possibilities are #, `, \, ~, and !. I think only backtick is > > completely unused, but the others should be pretty unambiguous when > > embedded in an identifier. > > Gabe Black wrote: > There was an instance of a sw field and an sw type which showed up even > now that accessing members with . is so cumbersome. Really, though, I think > using . for both is not only potentially ambiguous, it's also going to be > really confusing. People won't be able to look at a bit of code, say > Control.udw, and know for sure without looking something up if udw is a type > or a member. Also the regular expressions in, say, the reg_or_imm sort of > multiplexing instruction formats don't know what the type suffixes are. They > just accept anything like they did before, but it happens to work out. > > Even though _ may at first seem like it would blend in too much, and/or > be ambiguous, I think after looking at some examples it isn't really. > Historically operand names have been capitalized, so putting a _ is a clear > separator. It's also subtle enough to not obscure the name itself. I actually > had a few ISAs converted over to using @ before, and it was just so ugly I > gave up on it. You basically end up with this big wart in the middle of > everything where . or _ stay out of the way. > > Lets say you have a register XMM in x86 which is a, say, 64 bit floating > point register. Normally those are 128 bits, but I split them up into 64 > bits. But in any case, lets say the default type is a double, but you want it > to be a structure/union which is broken into lanes of various sizes. The type > suffix for that could be lns, and the 2 byte lanes could be, for instance, > wa, wb, wc, and wd. To store 0xaa55 into the third 2 byte word of the XMM > register, it would look like this. > > XMM_lns.wc = 0xaa55; > > I think that's clear for a couple reasons. First, it's not uncommon to > use a suffix to distinguish between different versions of the same thing with > the same name, and that's essentially what _lns is doing. Second, "." means > membership in C and C++ and python, etc., and here it does too. You're > accessing the wc member, not essentially doing a cast (or really selecting a > variant). If you had a control register and you saw. > > Control.udw = 1 > > And you weren't already programmed to assume that was a type override, > your initial assumption would likely be that you were assigning to the udw > field of the control register. In some cases, that may be exactly what you > want to do. If you have a (recently demoted) Twin64_t operand like, say, Mem > after a load, you could get at its a and b members like this. > > Ra = Mem_tud.a; > Rb = Mem_tud.b; > > which is also pretty clear. > > Another nice property of "_" compared to other punctuation, in addition > to it being aesthetically nice, is that it can't be confused for an operator > or break up a variable name. You could, theoretically, leave the code > untouched and it would still be valid code. You can't for logistical reasons > I mention in my commit message, but, otherwise it would work just fine. You > could copy and paste the code into your own file, and it would just work. > This also means the operand detector can be a little dumber because it > doesn't have to worry about accidentally thinking Ra/Rb means Ra but > instantiated as an Rb type operand. > > I thought about this quite a bit and it wasn't obvious that this would > work out very well, but after some thought and experimentation I think it > does.
I still don't think the "." is confusing, if you think of unions rather than structs. If you have a union of structs, then having a sequence of dotted elements is the obvious way to do it: XMM.lns.wc = 0xaa55. You can argue that unions are confusing, but then you're arguing with K&R and not me ;-). Or do you think that the type specifiers behave insufficiently like unions? Visually I think ":" looks pretty nice, but I can imagine that there are benefits in having something that's still a syntactically legal LHS expression (which either "." or "_" satisfies, but not much else). OTOH, maybe having something that's not legal C is a plus in that it makes it obvious to the reader that there's something magic going on. Not trying to nitpick you on this one, but it seems like a pretty significant change so I want to feel confident that we're going the right direction here. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/873/#review1547 ----------------------------------------------------------- On 2011-09-20 22:59:24, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/873/ > ----------------------------------------------------------- > > (Updated 2011-09-20 22:59:24) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ISA parser: Use '_' instead of '.' to delimit type modifiers on operands. > > By using an underscore, the "." is still available and can unambiguously be > used to refer to members of a structure if an operand is a structure, class, > etc. This change mostly just replaces the appropriate "."s with "_"s, but > there were also a few places where the ISA descriptions where handling the > extensions themselves and had their own regular expressions to update. The > regular expressions in the isa parser were updated as well. It also now > looks for one of the defined type extensions specifically after connecting "_" > where before it would look for any sequence of characters after a "." > following an operand name and try to use it as the extension. This helps to > disambiguate cases where a "_" may legitimately be part of an operand name but > not separate the name from the type suffix. > > Because leaving the "_" and suffix on the variable name still leaves a valid > C++ identifier and all extensions need to be consistent in a given context, I > considered leaving them on as a breadcrumb that would show what the intended > type was for that operand. Unfortunately the operands can be referred to in > code templates, the Mem operand in particular, and since the exact type of Mem > can be different for different uses of the same template, that broke things. > > > Diffs > ----- > > src/arch/alpha/isa/decoder.isa dee1f3ab92e4 > src/arch/alpha/isa/int.isa dee1f3ab92e4 > src/arch/alpha/isa/main.isa dee1f3ab92e4 > src/arch/arm/isa/insts/branch.isa dee1f3ab92e4 > src/arch/arm/isa/insts/data.isa dee1f3ab92e4 > src/arch/arm/isa/insts/div.isa dee1f3ab92e4 > src/arch/arm/isa/insts/fp.isa dee1f3ab92e4 > src/arch/arm/isa/insts/ldr.isa dee1f3ab92e4 > src/arch/arm/isa/insts/macromem.isa dee1f3ab92e4 > src/arch/arm/isa/insts/mem.isa dee1f3ab92e4 > src/arch/arm/isa/insts/misc.isa dee1f3ab92e4 > src/arch/arm/isa/insts/mult.isa dee1f3ab92e4 > src/arch/arm/isa/insts/neon.isa dee1f3ab92e4 > src/arch/arm/isa/insts/str.isa dee1f3ab92e4 > src/arch/arm/isa/insts/swap.isa dee1f3ab92e4 > src/arch/isa_parser.py dee1f3ab92e4 > src/arch/mips/isa/decoder.isa dee1f3ab92e4 > src/arch/mips/isa/formats/fp.isa dee1f3ab92e4 > src/arch/mips/isa/formats/int.isa dee1f3ab92e4 > src/arch/mips/isa/formats/mem.isa dee1f3ab92e4 > src/arch/power/isa/decoder.isa dee1f3ab92e4 > src/arch/power/isa/formats/fp.isa dee1f3ab92e4 > src/arch/sparc/isa/base.isa dee1f3ab92e4 > src/arch/sparc/isa/decoder.isa dee1f3ab92e4 > src/arch/x86/isa/microops/fpop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/ldstop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/limmop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/mediaop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/regop.isa dee1f3ab92e4 > > Diff: http://reviews.m5sim.org/r/873/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
