Andrew, My $0.2
It might be better to refactor switch(tag) in c1_LIRGenerator_aarch64.cpp to usual case/break form, rather than have multiple returns. i.e. switch (tag) { 583 case floatTag: // fall trough 584 case doubleTag: do_ArithmeticOp_FPU(x); break; 585 case longTag: do_ArithmeticOp_Long(x); break; 586 case intTag: do_ArithmeticOp_Int(x); break; 587 default: ShouldNotReachHere(); 588 } return; -Dmitry On 27.09.2018 11:53, Andrew Haley wrote: > On 09/27/2018 09:30 AM, Aleksey Shipilev wrote: >> On 09/27/2018 10:21 AM, Andrew Haley wrote: >>> On 09/27/2018 08:22 AM, Aleksey Shipilev wrote: >>>> Otherwise looks good and trivial. >>> >>> No, it is neither good nor trivial:some of those should be >>> ShouldNotReachHere(). >>> I'll post a webrev later. >> >> Okay, let's see it. >> >> I was mostly concerned with having the same control flow as before, >> did I miss some change that is actually non-trivial? > > That's not my point: we should not put break statements in places we > don't expect to reach. It's misleading for the reader. Code changes > to "shut up" the compiler have do be done with great care and > knowledge of what the code does. > >> On the second read, this change in c1_LIRAssembler_aarch64.cpp looks >> suspicious, as it elevates ShouldNotReachHere to default case, >> rather than letting default thing fall-through? >> >> break; >> + default: >> ShouldNotReachHere(); >> } > > I think that one is actually OK. > > http://cr.openjdk.java.net/~aph/8211207/ > > follows the pattern of the x86 port. > -- Dmitry Samersoff http://devnull.samersoff.net * There will come soft rains ...