This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner][AMDGPU][Mips] Fold bitcast with volatile loads if the resulting load is legal for the target.
ClosedPublic

Authored by craig.topper on Aug 8 2018, 11:04 PM.

Details

Summary

I'm not sure if this patch is correct or if it needs more qualifying somehow. Bitcast shouldn't change the size of the load so it should be ok? We already do something similar for stores. We'll change the type of a volatile store if the resulting store is Legal or Custom. I'm not sure we should be allowing Custom there...

I was playing around with converting X86 atomic loads/stores(except seq_cst) into regular volatile loads and stores during lowering. This would allow some special RMW isel patterns in X86InstrCompiler.td to be removed. But there's some floating point patterns in there that didn't work because we don't fold (f64 (bitconvert (i64 volatile load))) or (f32 (bitconvert (i32 volatile load))).

Diff Detail

Event Timeline

craig.topper created this revision.Aug 8 2018, 11:04 PM
arsenm added a subscriber: arsenm.Aug 8 2018, 11:12 PM

I've never understood the type legalization policy for volatile. It's never made any sense to me why it would change how loads and stores to be legalized based on the type, so this seems like an improvement to me

For volatile loads/stores, we should probably try to preserve the number and addresses of memory operations. If the source and destination types would lower to equivalent memory operations, like i32 vs. f32 on x86, the transform is fine. If they wouldn't, like f64 vs. i64 on x86-32, the transform is dubious; memory-mapped I/O can respond in strange ways if you don't use the right access width.

So the isOperationLegal check here should prevent (i64 (bitcast (f64 volatile load))) from being changed on x86-32. Do I need to make sure sure that i64 is legal to prevent (f64 (bitcast (i64 volatile load))) from being changed? The comments from r52254 where this check was originally added seems to suggest that its bad to increase the number of operations and that the number of operations needed for an illegal type is "undefined".

I guess we could assume anyone using memory-mapped hardware will only do it with legal types, so reducing the number of operations for illegal types doesn't matter. But please add a comment here (and to the related store transform) explaining that.

nhaehnle removed a subscriber: nhaehnle.Aug 27 2018, 6:35 AM
This revision is now accepted and ready to land.Aug 27 2018, 7:13 PM
This revision was automatically updated to reflect the committed changes.