[PATCH] D27783: [MachineBlockPlacement] Don't make blocks "uneditable"

[Sanjoy Das via Phabricator via llvm-commits]

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.


https://reviews.llvm.org/D27783