[PATCH] D27783: [MachineBlockPlacement] Don't make blocks "uneditable"
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 14 16:50:57 PST 2016
sanjoy added a comment.
In https://reviews.llvm.org/D27783#623014, @iteratee wrote:
> I'm OK with the debugging, but I think it's a little over-engineered. Basically you're keeping a list of all the blocks that you think have unanalyzable fallthrough and then you want to assert if during updateTerminators you encounter a block with unanalyzable fallthrough that is not on that list. Is that correct? If so simply keeping that set and testing for membership is sufficient, and simpler.
Sounds good -- I'll move the set to live on the MachineFunctionPass object then.
> I'm also OK removing the teeth in TailDuplicator.
> It bothers me that analyzeBranch has that flag at all.
Me too. :)
> My real surprise moment is that analyzeBranch would choose to modify a branch that it claims it can't analyze. Really! I'm not convinced that's correct, but I'm fine with some defensive programming in response to it.
I agree that's hard to defend, and I was initially considering fixing `analyzeBranch` to edit a branch only if it would return `false`. However, it is also reasonable to also say that the transform it did here is a pretty obvious peephole transform which it _should_ be able to do without fully understanding the branch.
I think an overall better solution is to have two TII hooks, `analyzeBranch` and `simplifyBranch`. `simplifyBranch` can then be specified to not indicate anything about the understandability of the terminator sequence -- its job would be to do locally obvious peephole simplification without any global understanding.
More information about the llvm-commits