- 
                Notifications
    You must be signed in to change notification settings 
- Fork 728
xip: Lookup float constants from table to reduce relocations #894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Merge bytecodealliance:main into wenyongh:refine_xip
|  | ||
| #if defined(BH_PLATFORM_WINDOWS) | ||
| static bool | ||
| str2uint32(const char *buf, uint32 *p_res) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between str2uint32 to the strtol from libc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lum1n0us  Seems the result of str2uint32 is a raw hex rather than an integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is raw int bits of f32 and f64.
        
          
                core/iwasm/compilation/aot_llvm.c
              
                Outdated
          
        
      | const_ptr_type = INT64_PTR_TYPE; | ||
| break; | ||
| case VALUE_TYPE_F32: | ||
| snprintf(buf, sizeof(buf), "i32#%08X", value->i32); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| snprintf(buf, sizeof(buf), "i32#%08X", value->i32); | |
| snprintf(buf, sizeof(buf), "i32#%08X", value->f32); | 
Seems there should be f32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be i32, store the raw int bits to make it simple and avoid data loss (possible data loss when using f32, e.g. NaN values).
| } | ||
|  | ||
| memset(sym, 0, sizeof(AOTNativeSymbol)); | ||
| snprintf(sym->symbol, sizeof(sym->symbol), "%s", symbol); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an assert to ensure the symbol is no longer than the buffer?
bh_assert(strlen(symbol) <= sizeof(sym->symbol));There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
        
          
                core/iwasm/aot/aot_loader.c
              
                Outdated
          
        
      | set_error_buf_v(error_buf, error_buf_size, | ||
| "missing native symbol: %s", symbol); | ||
| goto fail; | ||
| if (!strncmp(symbol, "i32#", 4)) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may let i32 be renamed n32. less confusion over i representing integer and f representing float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has renamed to "f32#"
| @wenyongh | 
| @yamt it depends, not sure how much footprint it can reduce and how much performance it will impact? I remember that many x86 instructions directly encode i32 immediate as its operand, the performance should be better than that of looking up i32 immediate from the constant table. | 
| 
 i'm not sure how much impact it is. | 
| Is it only required by XIP mode? For non-XIP, no need to eliminate these relocations, right? Seems no other good idea except do the similar for i32.const. | 
| 
 yes 
 ok | 
| @yamt Could you share more info (disassembly code, etc) about which instruction will lead to relocation in your application ? | 
| 
 just "hello world" using printf. for example, a constant 100000000 (00e1f505) is loaded from .rodata.cst4, it seems. maybe this one in   | 
…ealliance#894) Lookup float/double constants from exec_env->native_symbol table but not construct them with LLVMBuildConst if XIP mode is enabled, these constants are introduced by f32/f64.const opcodes and some float/double conversion opcodes, and make wamrc generate some relocations in text section of AOT XIP file. This patch eliminates such relocations when "--enable-indirect-mode" is added to wamrc.
Lookup float/double constants from exec_env->native_symbol table
but not construct them with LLVMBuildConst if XIP mode is enabled,
these constants are introduced by f32/f64.const opcodes and some
float/double conversion opcodes, and make wamrc generate some
relocations in text section of AOT XIP file. This patch eliminates such
relocations when "--enable-indirect-mode" is added to wamrc.