Disable sign extension in SignExtLowering.cpp #5676
Conversation
The sign extension lowering pass would previously lower away the sign extension instructions, but it wouldn't disable the sign extension feature, so follow-on passes such as optimize-instructions could reintroduce sign extension instructions. Fix the pass to disable the sign extension feature to prevent sign extension instructions from being reintroduced later.
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| @@ -68,6 +68,14 @@ struct SignExtLowering : public WalkerPass<PostWalker<SignExtLowering>> { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| void run(Module* module) override { | |||
| if (!module->features.has(FeatureSet::SignExt)) { | |||
There was a problem hiding this comment.
Is it possible the module could contain sign ext instructions but this feature not be enabled? (I guess in that case it would not validate?)
There was a problem hiding this comment.
I also wonder why the test we have in emscripten for this was passing.. investigating.
There was a problem hiding this comment.
The emscripten test merely checks that --signext-lowering is part of the wasm-opt command line .. I didn't both to actually decompile the module to verify binaryen was actually doing the right thing.
There was a problem hiding this comment.
Is it possible the module could contain sign ext instructions but this feature not be enabled? (I guess in that case it would not validate?)
Right, in this case the module would not validate and Binaryen would produce an error.
|
Wow! Thank you very much for this fast fix! :) |
|
Is there some context for this? I'm confused as to how it occurs. Depending on the context, perhaps it makes sense to have a new pass to remove the feature? I'm not sure we always want to do the two operations together. |
|
If we do want to bundle the operations, it might also be good to document that in the pass description perhaps. |
|
Context is emscripten-core/emscripten#19121. Basically this pass didn't do what it was supposed to when run before other passes like optimize-instructions. When would we ever want to lower away the sign extension instructions but allow them to be reintroduced later? |
|
I've now updated the pass description. |
|
I can imagine testing/fuzzing situations where we might want to lower certain instructions but maybe generate more of them later (could be more efficient than otherwise). In particular, the fuzzer assumes features stay the same throughout the pipeline, though I don't know if there is any danger to that assumption changing. And I agree that the obvious use case is to lower and disable the feature. Looks like in existing lowering passes we do that in MultiMemory, for the reason of avoiding the feature appearing in the features section later, but we don't do it for Memory64. We should probably make a general decision and be consistent about it. I don't have strong feelings so we could separately adjust Memory64 as well to disable the feature. |
|
I agree we should be consistent. I can update the Memory64 lowering pass as well. |
* Disable sign extension in SignExtLowering.cpp The sign extension lowering pass would previously lower away the sign extension instructions, but it wouldn't disable the sign extension feature, so follow-on passes such as optimize-instructions could reintroduce sign extension instructions. Fix the pass to disable the sign extension feature to prevent sign extension instructions from being reintroduced later. * update pass description
The sign extension lowering pass would previously lower away the sign extension
instructions, but it wouldn't disable the sign extension feature, so follow-on
passes such as optimize-instructions could reintroduce sign extension
instructions.
Fix the pass to disable the sign extension feature to prevent sign extension
instructions from being reintroduced later.