>> Also, please try to stick with the established style when modifying >> existing code. In this case, putting the '*' in the right place, >> capitalizing variables, etc. >> > Old habits die hard. I've had it drilled into my head for decades that > putting the * next to the var name is the *wrong* place to put it > as the > * modifies the type not the var. I'll try to retain style consistency, > however.
I agree with the theoretical idea behind that style, but... :) >> The cast sequence *is* needed for converting stuff to shifts etc, >> above. > > I'm not following this comment. What cast sequence? The one where > we're > trying to turn it into a udiv if the sign bits are zero? If so, that > contradicts your simplification for this transform. The specific transform this was attached to get simpler (no casts) but others get temporarily nastier (new casts) until other operations become signless. >> in the commonIDivTransforms method, but it is specific to udiv. >> Please move it to the udiv case, and make it insert casts etc as >> needed. > > Actually, I think I lost it completely. I don't see this in my file. I > added it back to the udiv case as a separate transform. It now checks > for the STO==0 and SFO==0 cases because they are no longer "handled > above" (the "above" code is in commonDivTransforms). Ok. >> *** This is subtly buggy. If you look at how MulWithOverflow is >> used, >> it is called from the "Fold: (div X, C1) op C2 -> range check" case. >> You need to pass in whether or not the *operation* is signed or >> unsigned, ignoring the sign of the values. > > MulWithOverflow isn't used elsewhere in the file so I merged it into > visitSelectCC so its a little more clear what's going on. No point > making a function with 4 arguments called from only one place. Ok, though sometimes names do give useful info to the reader. A comment does just the same though. Thx Reid, -Chris _______________________________________________ llvm-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
